Add nullptr support to scoped_ptr.

Add nullptr support to scoped_ptr.

This adds support to use nullptr to construct, assign, or return a
scoped_ptr<T> and scoped_ptr<T[]>. Support for this requires the use
of a move-only constructor.

The changes are:

- Add a constructor that takes decltype(nullptr) as a parameter. This
allows behaviour such as scoped_ptr<T>(nullptr), but also allows a
function with return type scoped_ptr<T> to "return nullptr;" instead
of "return scoped_ptr<T>();".

- Add an operator=(decltype(nullptr)) that resets the scoped_ptr to
empty and deletes anything it held.

- Add/Modify a constructor to take a scoped_ptr<U,E>&& parameter for
constructing a scoped_ptr from another using move-only semantics. This
piece is critical for allowing the function returning nullptr to be
assigned to some other scoped_ptr at the callsite. In particular, take
the following code:
  scoped_ptr<T> Function() { return nullptr; }
  scoped_ptr<T> var = Function();
In this case the constructor which takes a nullptr allows Function() to
be written, but not to be used. The move-only constructor allows the
assignment from Function() to var. See "C++11 feature proposal:
Move-only constructors" on chromium-dev for more explanation why.

The scoped_ptr<T> class already had a constructor which took
scoped_ptr<U,E> as an argument, so this was changed to be
scoped_ptr<U,E>&& instead. The scoped_ptr<T[]> class had no such
constructor, so a scoped_ptr&& constructor was added. These match
the constructors found on the unique_ptr class.

- Remove the RValue type and the contructor that constructs a
scoped_ptr from an RValue. Change Pass() to return a scoped_ptr&&
instead of a scoped_ptr::RValue, to avoid the type conversion and
remove some complexity. This is done with a new emulation macro that
still provides Pass() and makes the type go down the MoveOnlyType
path in base::Callback code.

This adds base_unittests to demonstrate and use these changes.

The use of Pass() remains unchanged until std::move() is written
or allowed. At that time std::move() could be used instead of Pass.

R=brettw@chromium.org, jamesr@chromium.org

Committed: https://crrev.com/2299e91d3508f8d5d18ef990cf6024ea4371250a
Cr-Commit-Position: refs/heads/master@{#297072}

Committed: https://crrev.com/a9527ce329c38d945e46773f1592a4939cf62b99
Cr-Commit-Position: refs/heads/master@{#297116}

[danakj, 2014-09-25 01:11:01]

This hasn't been formally allowed yet, but if it were...

[Nico, 2014-09-25 01:58:42]

Out of curiosity: For the third point, would it be possible to change the
CPP03_MOVE_EMULATION (spelling?) to do that, so that this works for all types
that currently use move emulation?

[danakj, 2014-09-25 02:22:19]

On Sep 24, 2014 9:58 PM, <thakis@chromium.org> wrote:
>
> Out of curiosity: For the third point, would it be possible to change the
> CPP03_MOVE_EMULATION (spelling?) to do that, so that this works for all
types
> that currently use move emulation?

It's complicated. For instance the constructors are different between
scoped_ptr<T> and T[], where one is templates and one isn't. I could add a
non-templated move-only constructor, that relied on an operator= on the
using class, but that might slow down the build? (I assume that's why we
omitted it on scoped_ptr.)

And I'm not sure that all users of the macro have an operator= or want one
for the macro to use, could add a private one but it'd make this CL larger.
Do you see a nice way to add a move constructor to the macro I'm missing?

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2014-09-25 02:44:53]

One more thought. This move constructor is only needed with our emulation
in places where we add a constructor from a type other than RValue and want
to return that type (nullptr). So making it useful means adding a
constructor to other classes already and I'm pretty sure we don't want to
use nullptr for all move emulated classes.. File comes to mind.
On Sep 24, 2014 10:22 PM, "Dana Jansens" <danakj@chromium.org> wrote:

>
> On Sep 24, 2014 9:58 PM, <thakis@chromium.org> wrote:
> >
> > Out of curiosity: For the third point, would it be possible to change the
> > CPP03_MOVE_EMULATION (spelling?) to do that, so that this works for all
> types
> > that currently use move emulation?
>
> It's complicated. For instance the constructors are different between
> scoped_ptr<T> and T[], where one is templates and one isn't. I could add a
> non-templated move-only constructor, that relied on an operator= on the
> using class, but that might slow down the build? (I assume that's why we
> omitted it on scoped_ptr.)
>
> And I'm not sure that all users of the macro have an operator= or want one
> for the macro to use, could add a private one but it'd make this CL larger.
> Do you see a nice way to add a move constructor to the macro I'm missing?
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jamesr, 2014-09-25 05:58:08]

jamesr@chromium.org changed reviewers:
+ jamesr@chromium.org

[jamesr, 2014-09-25 05:58:09]

I think in the future we'll want to allow assigning a scoped_ptr<U>&& to a
scoped_ptr<T> if U and T are compatible, but we don't need that today.

I think this is pretty reasonable to support, personally.

https://codereview.chromium.org/599313003/diff/1/base/memory/scoped_ptr.h
File base/memory/scoped_ptr.h (right):

https://codereview.chromium.org/599313003/diff/1/base/memory/scoped_ptr.h#new...
base/memory/scoped_ptr.h:322: scoped_ptr() : impl_(NULL) {}
nit: impl_(nullptr) ?

https://codereview.chromium.org/599313003/diff/1/base/memory/scoped_ptr.h#new...
base/memory/scoped_ptr.h:336: // IMPLEMENTATION NOTE: C++11 unique_ptr<> keeps
this constructor distinct
does this note still apply?

https://codereview.chromium.org/599313003/diff/1/base/memory/scoped_ptr.h#new...
base/memory/scoped_ptr.h:467: scoped_ptr() : impl_(NULL) {}
s/NULL/nullptr/, same for the rest of file

https://codereview.chromium.org/599313003/diff/1/base/memory/scoped_ptr.h#new...
base/memory/scoped_ptr.h:477: // - it cannot be NULL, because NULL is an
integral expression, not a
update this part of the comment plz

[danakj, 2014-09-25 14:47:52]

PTAL

https://codereview.chromium.org/599313003/diff/1/base/memory/scoped_ptr.h
File base/memory/scoped_ptr.h (right):

https://codereview.chromium.org/599313003/diff/1/base/memory/scoped_ptr.h#new...
base/memory/scoped_ptr.h:336: // IMPLEMENTATION NOTE: C++11 unique_ptr<> keeps
this constructor distinct
On 2014/09/25 05:58:08, jamesr wrote:
> does this note still apply?

I think so. unique_ptr has 2 constructors:
unique_ptr(unique_ptr&&)
template <U,E> unique_ptr(unique_ptr<T,E>&&)

Where as scoped_ptr just has the latter. I believe that's what this comment is
about.

[danakj, 2014-09-25 14:50:19]

https://codereview.chromium.org/599313003/diff/1/base/memory/scoped_ptr.h
File base/memory/scoped_ptr.h (right):

https://codereview.chromium.org/599313003/diff/1/base/memory/scoped_ptr.h#new...
base/memory/scoped_ptr.h:336: // IMPLEMENTATION NOTE: C++11 unique_ptr<> keeps
this constructor distinct
On 2014/09/25 14:47:52, danakj wrote:
> On 2014/09/25 05:58:08, jamesr wrote:
> > does this note still apply?
> 
> I think so. unique_ptr has 2 constructors:
> unique_ptr(unique_ptr&&)
> template <U,E> unique_ptr(unique_ptr<T,E>&&)

Rather,

unique_ptr(unique_ptr&&)
template <U,E> unique_ptr(unique_ptr<U,E>&&)

> Where as scoped_ptr just has the latter. I believe that's what this comment is
> about.

[jamesr, 2014-09-25 16:14:25]

This lgtm, although I'm not an owner in these parts.

[danakj, 2014-09-25 16:38:21]

brettw: can you OWNERS review?

I believe nico is out atm.

[brettw, 2014-09-25 21:31:52]

LGTM. It makes sense, I'm hoping the others did a careful check of the gotchas
since I'm not very experienced with rvalue refs yet.

[danakj, 2014-09-26 02:02:03]

jamesr: PTAL

Here's a slightly simpler version, where I was able to remove the RValue nested
type from scoped_ptr and the conversions to RValue and back to scoped_ptr when
doing a Pass. This could actually make things faster too.

I added another CPP03 emulation macro for classes that do things this way. It
still defines Pass() and the MoveOnlyTypeForCPP03 typedef so that base::Bind
works.

[danakj, 2014-09-26 03:19:58]

+ajwong FYI cuz it'll make you happy (I hope)

[jamesr, 2014-09-26 04:43:57]

I preferred patch set 2.  static_cast<T&&>(..) is not exactly the same thing as
std::move() and I'm not aware of all of the consequences of the differences.

[awong, 2014-09-26 04:51:45]

On 2014/09/26 04:43:57, jamesr wrote:
> I preferred patch set 2.  static_cast<T&&>(..) is not exactly the same thing
as
> std::move() and I'm not aware of all of the consequences of the differences.

@danakj: Have you matched this up with the unique_ptr<> implementation? At this
point, the two should be nearly identical.

[danakj, 2014-09-26 13:40:46]

On 2014/09/26 04:51:45, awong wrote:
> @danakj: Have you matched this up with the unique_ptr<> implementation? At
this
> point, the two should be nearly identical.

Yes, I've been basing this change off of unique_ptr here
http://llvm.org/viewvc/llvm-project/libcxx/trunk/include/memory?view=markup ,
but here's an audit.

Things unique_ptr has that scoped_ptr doesn't:
- unique_ptr(pointer p, see below d1) noexcept;  // scoped_ptr has
scoped_ptr(element_type* p, const D& d)
- unique_ptr(pointer p, see below d2) noexcept;  // scoped_ptr has
scoped_ptr(element_type* p, const D& d)
- unique_ptr(unique_ptr&& u) noexcept;  // scoped_ptr has only templated version
- template <class U> unique_ptr(auto_ptr<U>&& u) noexcept;  // scoped_ptr does
not support auto_ptr
- unique_ptr& operator=(unique_ptr&& u) noexcept;  // scoped_ptr has only
templated version
- explicit operator bool() const noexcept;  // scoped_ptr has operator Testable
- bool operator<(...)
- bool operator<=(...)
- bool operator>(...)
- bool operator>=(...)
- bool operator==(...)  // scoped_ptr has this, but unique_ptr allows comparison
of convertible types
- bool operator!=(...)  // scoped_ptr has this, but unique_ptr allows comparison
of convertible types

Things scoped_ptr has that unique_ptr doesn't:
- typedef void MoveOnlyTypeForCPP03;  // for Callback implementation
- type Pass()  // to replace std::move()
- type PassAs()  // legacy not needed anymore

[danakj, 2014-09-26 13:57:36]

On 2014/09/26 04:43:57, jamesr wrote:
> I preferred patch set 2.  static_cast<T&&>(..) is not exactly the same thing
as
> std::move() and I'm not aware of all of the consequences of the differences.

Actually, I believe that foo.Pass() is now what std::move(foo) would resolve to,
with one difference. Let me explain.

Here's the def'n of std::move from
http://llvm.org/viewvc/llvm-project/libcxx/trunk/include/type_traits?view=markup

template <class T> typename remove_reference<T>::type&& move(T&& t) {
  typedef typename remove_reference<T>::type _Up;
  return static_cast<_Up&&>(t);
}

Now consider scoped_ptr<W> sp; here's what std::move(sp) would look like with
resolved types

typename remove_reference<scoped_ptr<W>&>::type&& move(scoped_ptr<W>& && t) {
  typedef typename remove_reference<scoped_ptr<W>&>::type _Up;
  return static_cast<_Up&&>(t);
}

The remove_reference<scoped_ptr<W>&> can be resolved to scoped_ptr<W>
everywhere, and & && to &, giving:

scoped_ptr<W>&& move(scoped_ptr<W>& t) {
  typedef scoped_ptr<W> _Up;
  return static_cast<_Up&&>(t);
}

Or..

scoped_ptr<W>&& move(scoped_ptr<W>& t) {
  return static_cast<scoped_ptr<W>&&>(t);
}


Now compare to what sp.Pass() would be:

scoped_ptr<W> scoped_ptr<W>::Pass() {
  return static_cast<scoped_ptr<W>&&>(*this);
}

The difference is now just the return type of Pass() which is scoped_ptr, but
std::move would return scoped_ptr&&.

What this translates to is that an rvalue scoped_ptr is constructed at the
callsite of sp.Pass() - the same as it has been done previously. This means the
data inside sp gets stolen and moved into the newly constructed rvalue object at
the callsite. ScopedPtrTest.PassBehavior verifies this behaviour with this test:

  {
    ConDecLogger* logger = new ConDecLogger(&constructed);
    scoped_ptr<ConDecLogger> scoper(logger);
    EXPECT_EQ(1, constructed);

    // Should auto-destruct logger by end of scope.                             
                                                                  
    scoper.Pass();
    EXPECT_FALSE(scoper.get());
  }

The scoper.get() is nullptr there, because Pass() has made a new rvalue
scoped_ptr already. If Pass() is changed to return scoped_ptr&& then it emulates
move() more closely, but changes from existing behaviour more - and I would need
to update this test.

[danakj, 2014-09-26 14:04:18]

To make Pass() exactly the same as std::move(), the test before:

  {
    ConDecLogger* logger = new ConDecLogger(&constructed);
    scoped_ptr<ConDecLogger> scoper(logger);
    EXPECT_EQ(1, constructed);

    // Should auto-destruct logger by end of scope.                             
                                                                  
    scoper.Pass();
    EXPECT_FALSE(scoper.get());
  }
  EXPECT_EQ(0, constructed);

And after:

  {
    ConDecLogger* logger = new ConDecLogger(&constructed);
    scoped_ptr<ConDecLogger> scoper(logger);
    EXPECT_EQ(1, constructed);

    // Should auto-destruct logger by end of scope.                             
                                                                  
    scoper.Pass();
  }
  EXPECT_EQ(0, constructed);

The scoped is no longer modified at the callsite of Pass(), but constructed is
still verified to be 0, so that a leak did not occur. I've updated the CL to
this and to make Pass() exactly the same as std::move().

[danakj, 2014-09-26 14:52:57]

Patchset #5 (id:80001) has been deleted

[danakj, 2014-09-26 15:05:25]

Patchset #5 (id:100001) has been deleted

[danakj, 2014-09-26 15:05:58]

Patchset #5 (id:120001) has been deleted

[jamesr, 2014-09-26 20:22:45]

I believe I understand patch set 2.  I do not understand patch set 5.  It's
possible that I will understand with enough time, but what exactly does patch
set 5 allow that patch set 2 does not?  Are there bugs that are possible to
write with patch set 2 that aren't possible in patch set 5?

I'm fine with having some restrictions in place that std::unique_ptr<> do not
have.  It's always possible to allow more things in the future.  I'm less fine
with having something here that we don't fully understand since we're on our own
here.  When we upgrade to using std::unique_ptr<>, we can depend on lots of
people who understand this stuff having reviewed the implementation carefully
and lots of other people who use the standard library we use having tested it
thoroughly.

https://codereview.chromium.org/599313003/diff/140001/base/move.h
File base/move.h (right):

https://codereview.chromium.org/599313003/diff/140001/base/move.h#newcode224
base/move.h:224: type&& Pass() { return static_cast<type&&>(*this); } \
no, this is not something we should add as a general purpose macro yet.

[danakj, 2014-09-26 20:57:53]

Patchset 2 and 5 are identical for users of scoped_ptr, unless they are trying
to NULL-test scoped_ptrs that they have Pass()'d to nothing at all.

Patchset 5 has no temporary types constructed when passing a scoped_ptr.
Patchset 2 requires construction of an intermediate scoped_ptr that exists only
to be moved into the final destination (as does the 03 emulation).

What do you not understand about patchset 5, I'm happy to walk through anything
with you.

The difference is (when using .Pass()), old emulation:

- old.Pass() returns a scoped_ptr(RValue(old))
- object creation looks like this:

old.Pass() -> RValue(old) r -> scoped_ptr(RValue) t -> scoped_ptr(scoped_ptr)
result

Patchset 2:

- old.Pass() returns a scoped_ptr(RValue(old))
- object creation looks like this:

old.Pass() -> RValue(old) r -> scoped_ptr(RValue) t -> scoped_ptr(scoped_ptr&&)
result

Patchset 5:

- old.Pass() returns a scoped_ptr&& for old
- object creation looks like this:

old.Pass() -> scoped_ptr(scoped_ptr&&) result

https://codereview.chromium.org/599313003/diff/140001/base/move.h
File base/move.h (right):

https://codereview.chromium.org/599313003/diff/140001/base/move.h#newcode224
base/move.h:224: type&& Pass() { return static_cast<type&&>(*this); } \
On 2014/09/26 20:22:45, jamesr wrote:
> no, this is not something we should add as a general purpose macro yet.

Why? It's only useful if they also add a T&& constructor to the class, which is
banned. I could do all of this inline in the 2 scoped_ptr classes, but given the
aforementioned, is that really needed?

[jamesr, 2014-09-26 21:00:47]

On 2014/09/26 20:57:53, danakj wrote:
> Patchset 2 and 5 are identical for users of scoped_ptr, unless they are trying
> to NULL-test scoped_ptrs that they have Pass()'d to nothing at all.
> 
> Patchset 5 has no temporary types constructed when passing a scoped_ptr.
> Patchset 2 requires construction of an intermediate scoped_ptr that exists
only
> to be moved into the final destination (as does the 03 emulation).

OK, so does that matter?  Does it generate more code in an optimized build?

> https://codereview.chromium.org/599313003/diff/140001/base/move.h
> File base/move.h (right):
> 
> https://codereview.chromium.org/599313003/diff/140001/base/move.h#newcode224
> base/move.h:224: type&& Pass() { return static_cast<type&&>(*this); } \
> On 2014/09/26 20:22:45, jamesr wrote:
> > no, this is not something we should add as a general purpose macro yet.
> 
> Why? It's only useful if they also add a T&& constructor to the class, which
is
> banned. I could do all of this inline in the 2 scoped_ptr classes, but given
the
> aforementioned, is that really needed?

There's no point in adding a macro that is used exactly once.

[danakj, 2014-09-26 21:03:59]

On 2014/09/26 21:00:47, jamesr wrote:
> On 2014/09/26 20:57:53, danakj wrote:
> > Patchset 2 and 5 are identical for users of scoped_ptr, unless they are
trying
> > to NULL-test scoped_ptrs that they have Pass()'d to nothing at all.
> > 
> > Patchset 5 has no temporary types constructed when passing a scoped_ptr.
> > Patchset 2 requires construction of an intermediate scoped_ptr that exists
> only
> > to be moved into the final destination (as does the 03 emulation).
> 
> OK, so does that matter?  Does it generate more code in an optimized build?

Let me see! brb

> > https://codereview.chromium.org/599313003/diff/140001/base/move.h
> > File base/move.h (right):
> > 
> > https://codereview.chromium.org/599313003/diff/140001/base/move.h#newcode224
> > base/move.h:224: type&& Pass() { return static_cast<type&&>(*this); } \
> > On 2014/09/26 20:22:45, jamesr wrote:
> > > no, this is not something we should add as a general purpose macro yet.
> > 
> > Why? It's only useful if they also add a T&& constructor to the class, which
> is
> > banned. I could do all of this inline in the 2 scoped_ptr classes, but given
> the
> > aforementioned, is that really needed?
> 
> There's no point in adding a macro that is used exactly once.

There's 2 uses, scoped_ptr<T> and scoped_ptr<T[]>.

[danakj, 2014-09-26 21:52:37]

On 2014/09/26 21:03:59, danakj wrote:
> On 2014/09/26 21:00:47, jamesr wrote:
> > On 2014/09/26 20:57:53, danakj wrote:
> > > Patchset 2 and 5 are identical for users of scoped_ptr, unless they are
> trying
> > > to NULL-test scoped_ptrs that they have Pass()'d to nothing at all.
> > > 
> > > Patchset 5 has no temporary types constructed when passing a scoped_ptr.
> > > Patchset 2 requires construction of an intermediate scoped_ptr that exists
> > only
> > > to be moved into the final destination (as does the 03 emulation).
> > 
> > OK, so does that matter?  Does it generate more code in an optimized build?
> 
> Let me see! brb

I see no difference in the assembly being generated unless I Pass() without
something to catch it and try to use the variable. So the advantage to patchset
5 is it is much closer to unique_ptr, with Pass() as an alias for move(), but
performance should be the same, I think.

[jamesr, 2014-09-26 21:54:11]

OK, thank you for investigating.  I still prefer PS2.

[danakj, 2014-09-26 21:55:54]

Since I don't think I can convince you of the value of making this more like
unique_ptr, patch set 6 is patch set 2 with some added Conversion unit tests.

[danakj, 2014-09-26 21:56:09]

The CQ bit was checked by danakj@chromium.org

[danakj, 2014-09-26 21:56:19]

On 2014/09/26 21:54:11, jamesr wrote:
> OK, thank you for investigating.  I still prefer PS2.

kk, CQ+

[jamesr, 2014-09-26 21:58:26]

I think there's some value, but I don't think the value outweighs the risks
here.

https://codereview.chromium.org/599313003/diff/160001/base/memory/scoped_ptr_...
File base/memory/scoped_ptr_unittest.cc (left):

https://codereview.chromium.org/599313003/diff/160001/base/memory/scoped_ptr_...
base/memory/scoped_ptr_unittest.cc:409: EXPECT_FALSE(scoper.get());
does this test fail now?

[commit-bot: I haz the power, 2014-09-26 21:58:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/599313003/160001

[jamesr, 2014-09-26 21:58:57]

The CQ bit was unchecked by jamesr@chromium.org

[jamesr, 2014-09-26 21:59:10]

Sorry unchecking, I want to make sure we understand what's happening in that
test.

[danakj, 2014-09-26 22:00:09]

https://codereview.chromium.org/599313003/diff/160001/base/memory/scoped_ptr_...
File base/memory/scoped_ptr_unittest.cc (left):

https://codereview.chromium.org/599313003/diff/160001/base/memory/scoped_ptr_...
base/memory/scoped_ptr_unittest.cc:409: EXPECT_FALSE(scoper.get());
On 2014/09/26 21:58:26, jamesr wrote:
> does this test fail now?

Oh, this can stay now, thanks. I'm going to add a comment that this is different
from unique_ptr also while I'm here.

[danakj, 2014-09-26 22:01:25]

Added back:

// This differs from unique_ptr, as Pass() has side effects but std::move()     
                                                              
// does not.                                                                    
                                                              
EXPECT_FALSE(scoper.get());

[danakj, 2014-09-26 22:02:17]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-09-26 22:03:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/599313003/180001

[jamesr, 2014-09-26 22:41:03]

https://codereview.chromium.org/599313003/diff/180001/base/memory/scoped_ptr_...
File base/memory/scoped_ptr_unittest.cc (right):

https://codereview.chromium.org/599313003/diff/180001/base/memory/scoped_ptr_...
base/memory/scoped_ptr_unittest.cc:409: // This differs from unique_ptr, as
Pass() has side effects but std::move()
excellent! would you mind filing a crbug that describes this difference in
behavior so we can remember to be careful when we do switch to std::unique_ptr<>
and std::move()? I think to start adding a WARN_IF_UNUSED to
scoped_ptr<>::Pass() could be good, then we can see what else needs to be done.

[danakj, 2014-09-26 22:58:20]

Filed https://code.google.com/p/chromium/issues/detail?id=418297

[commit-bot: I haz the power, 2014-09-26 23:36:26]

Committed patchset #7 (id:180001) as b2a8bf07aab072d48e965c65275257a241ca8354

[commit-bot: I haz the power, 2014-09-26 23:37:21]

Patchset 7 (id:??) landed as
https://crrev.com/2299e91d3508f8d5d18ef990cf6024ea4371250a
Cr-Commit-Position: refs/heads/master@{#297072}

[dcheng, 2014-09-27 07:33:55]

dcheng@chromium.org changed reviewers:
+ dcheng@chromium.org

[dcheng, 2014-09-27 07:33:56]

This is causing several test failures:
https://build.chromium.org/p/chromium.memory/builders/Mac%20ASan%2064%20Tests...:
ScopedPtrTest.ScopedPtrWithArray (run #1):
[ RUN      ] ScopedPtrTest.ScopedPtrWithArray
=================================================================
==3530==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x6110000b9680
at pc 0x0001090f4b24 bp 0x7fff57113410 sp 0x7fff57113408
READ of size 8 at 0x6110000b9680 thread T0

https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%...:
https://build.chromium.org/p/chromium.linux/builders/Linux%20Tests%20(dbg)(2)...:
https://build.chromium.org/p/chromium.linux/builders/Linux%20Tests%20(dbg)(2)...:
ScopedPtrTest.NullptrArray (run #1):
[ RUN      ] ScopedPtrTest.NullptrArray
memory allocation/deallocation mismatch at 0x2668562c3360: allocated with new
being deallocated with delete []
Received signal 11 SEGV_MAPERR 000000000039

[dcheng, 2014-09-27 07:34:58]

A revert of this CL (patchset #7 id:180001) has been created in
https://codereview.chromium.org/603353005/ by dcheng@chromium.org.

The reason for reverting is: This CL broke several scoped_ptr tests..

[danakj, 2014-09-27 18:48:12]

Thanks for the links.

On Sat, Sep 27, 2014 at 3:33 AM, <dcheng@chromium.org> wrote:

> https://build.chromium.org/p/chromium.memory/builders/Mac%
> 20ASan%2064%20Tests%20(1)/builds/2337:
> ScopedPtrTest.ScopedPtrWithArray (run #1):
> [ RUN      ] ScopedPtrTest.ScopedPtrWithArray
> =================================================================
> ==3530==ERROR: AddressSanitizer: heap-buffer-overflow on address
> 0x6110000b9680
> at pc 0x0001090f4b24 bp 0x7fff57113410 sp 0x7fff57113408
> READ of size 8 at 0x6110000b9680 thread T0
>

This one isn't reproducable on linux it seems, I'll have to see what's
going on here on Mac.


> ScopedPtrTest.NullptrArray (run #1):
> [ RUN      ] ScopedPtrTest.NullptrArray
> memory allocation/deallocation mismatch at 0x2668562c3360: allocated with
> new
> being deallocated with delete []
>

This one is my mistake, i used new int instead of new int[] in the test I
added.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jamesr, 2014-09-27 18:56:52]

Hmm, looks like somehow this allowed converting between scoped_ptr<>s with
different deleter types.

[dcheng, 2014-09-27 19:07:14]

On 2014/09/27 at 18:56:52, jamesr wrote:
> Hmm, looks like somehow this allowed converting between scoped_ptr<>s with
different deleter types.

It looks like the Mac ASAN bot failure is something else. It's still failing on
the waterfall.

[danakj, 2014-09-27 19:12:13]

Maybe it's
https://chromium.googlesource.com/chromium/src/+/ad66334103b9b9a3129e98fc9dec...


Ok well I've fixed the new-vs-new[] thing in the last patch set, so I guess I
will re-CQ this.

[danakj, 2014-09-27 19:13:01]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-09-27 19:13:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/599313003/200001

[commit-bot: I haz the power, 2014-09-27 20:22:20]

Committed patchset #8 (id:200001) as ae3a5edbe51dceb5c33c63d9dbed8a4c87def403

[commit-bot: I haz the power, 2014-09-27 20:22:51]

Patchset 8 (id:??) landed as
https://crrev.com/a9527ce329c38d945e46773f1592a4939cf62b99
Cr-Commit-Position: refs/heads/master@{#297116}

[jamesr, 2014-09-27 20:30:36]

https://codereview.chromium.org/599313003/diff/200001/base/memory/scoped_ptr_...
File base/memory/scoped_ptr_unittest.cc (right):

https://codereview.chromium.org/599313003/diff/200001/base/memory/scoped_ptr_...
base/memory/scoped_ptr_unittest.cc:630: scoped_ptr<int[]> scoper2(new int[3]);
oh, that would explain it!  we do seem to have pretty good coverage above for
preserving deleters, but we might want to add more tests using rvalue semantics
when we get there.

[eustas, 2014-09-29 08:15:34]

A revert of this CL (patchset #8 id:200001) has been created in
https://codereview.chromium.org/604423005/ by eustas@chromium.org.

The reason for reverting is: This patch seems to break ScopedPtrWithArray ASAN
test.

https://build.chromium.org/p/chromium.memory/builders/Mac%20ASan%2064%20Tests....