Extend scoped_ptr to be closer to unique_ptr. Support custom deleters, and deleting arrays.

Extend scoped_ptr to be closer to unique_ptr. Support custom deleters, and deleting arrays.

This is based on the modifications by "Geoffrey Romer"
 <gromer@google.com>; to Google's internal version of scoped_ptr<>.  All cleaver tricks are his, not mine. :)

It brings most of the features of C++11's unique_ptr<> into this class and should eliminate the need for scoped_array<>, scoped_malloc_free<>, and possibly a few other scopers.  Please refer to the unique_ptr<> API for documentation.

Divergence from unique_ptr<>:
1) DefaultDeleter<T[n]> (sized-arrays) is explicitly disabled because this construct would cause the single-object delete to be called on a pointer to a T[n]. It is impossible to construct a single-object new expression that returns a T[n]*. You can only get this with new[] which should correspond to DefaultDeleter<T[n][]>. This issue has been raised with the C++ LWG as it is possible a unique_ptr<> spec defect.

2) Reference types for Deleters are not supported. This simplifies implementation significantly because there is no need to distinguish between the converting constructor + converting assignment operator and their move constructor + move assignment operator.

4) Move-only Deleters are not supported. Avoids the need to emulate std::forward().

BUG=109874
TBR=dhollowa,kinuko,ananta,hans,scherkus

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=174057

[awong, 2012-10-13 01:05:00]

THIS IS NOT READY FOR A REAL REVIEW

[awong, 2012-10-13 01:05:27]

but I wanted to send a draft to see if there are any early comments. It's a
half-port of gromer's work in google3 to unify all these things.

[Jeffrey Yasskin, 2012-10-13 01:33:29]

I haven't gotten all the way through this.

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

https://codereview.chromium.org/11149006/diff/2001/base/memory/scoped_ptr.h#n...
base/memory/scoped_ptr.h:97: #include <algorithm> // TODO(ajwong): Do we really
want std::swap?
Yeah, we need to include std::swap in the overload sets. C++11 says we can
#include <utililty>, which is lighter, but C++98 doesn't permit that. You might
try it anyway to see if our libraries let us.

https://codereview.chromium.org/11149006/diff/2001/base/memory/scoped_ptr.h#n...
base/memory/scoped_ptr.h:154: template <class C, class D>
I'd try for better names than "C" and "D".

https://codereview.chromium.org/11149006/diff/2001/base/memory/scoped_ptr.h#n...
base/memory/scoped_ptr.h:161: (static_cast<D&>(data_))(data_.ptr);
Give data_ a .deleter() method? Strictly, you could just call
data_(data_.ptr()), but that's confusing.

https://codereview.chromium.org/11149006/diff/2001/base/memory/scoped_ptr.h#n...
base/memory/scoped_ptr.h:173: // on the current sequencing. See
http://b/6987235.
Clean up b/ references before committing.

[awong, 2012-10-18 02:46:15]

Comments addressed.

Jeffrey, Ryan: There are a few places where I could really use feedback. See my
TODO() comments which call them out. In particular, the move semantics for the
deleter are something I'm not sure that we want to tackle.

After that, I need to write a few more tests and see if I can get all of chrome
to compile, but I think this is getting close.

http://codereview.chromium.org/11149006/diff/2001/base/memory/scoped_ptr.h
File base/memory/scoped_ptr.h (right):

http://codereview.chromium.org/11149006/diff/2001/base/memory/scoped_ptr.h#ne...
base/memory/scoped_ptr.h:97: #include <algorithm> // TODO(ajwong): Do we really
want std::swap?
On 2012/10/13 01:33:29, Jeffrey Yasskin wrote:
> Yeah, we need to include std::swap in the overload sets. C++11 says we can
> #include <utililty>, which is lighter, but C++98 doesn't permit that. You
might
> try it anyway to see if our libraries let us.

Fixed locally. Will be up in next patch.

http://codereview.chromium.org/11149006/diff/2001/base/memory/scoped_ptr.h#ne...
base/memory/scoped_ptr.h:154: template <class C, class D>
On 2012/10/13 01:33:29, Jeffrey Yasskin wrote:
> I'd try for better names than "C" and "D".

Done.

http://codereview.chromium.org/11149006/diff/2001/base/memory/scoped_ptr.h#ne...
base/memory/scoped_ptr.h:161: (static_cast<D&>(data_))(data_.ptr);
On 2012/10/13 01:33:29, Jeffrey Yasskin wrote:
> Give data_ a .deleter() method? Strictly, you could just call
> data_(data_.ptr()), but that's confusing.

Done.

http://codereview.chromium.org/11149006/diff/2001/base/memory/scoped_ptr.h#ne...
base/memory/scoped_ptr.h:173: // on the current sequencing. See
http://b/6987235.
On 2012/10/13 01:33:29, Jeffrey Yasskin wrote:
> Clean up b/ references before committing.

Removed.  Still need to file a bug, but first I want more confidence that this
unification can even go in.

[Ryan Sleevi, 2012-10-18 03:17:10]

Before you work too hard on getting it cleaned up - does it work on Windows?

MSVC has been a persistent thorn in my side throughout my career when dealing
with templates, so I'll be surprised if some of this works. I suspect the switch
to MSVC 2010 *may* finally have it doing something decent (MSVC 6/7/8 were
god-awful with template resolution), but I just want to make sure you're
iterating concurrently on the right platforms.

http://codereview.chromium.org/11149006/diff/14002/base/memory/scoped_ptr.h
File base/memory/scoped_ptr.h (right):

http://codereview.chromium.org/11149006/diff/14002/base/memory/scoped_ptr.h#n...
base/memory/scoped_ptr.h:165: template <class Element, class Deleter>
Why not keep the naming consistent with std::, in calling the type T instead.

http://codereview.chromium.org/11149006/diff/14002/base/memory/scoped_ptr.h#n...
base/memory/scoped_ptr.h:180: get_deleter() = other.get_deleter();
So I'm not misreading - you're intentionally relying on type punning (shearing?
stripping) to ensure that |data_.ptr| is not copied over (since it's being
explicitly cast to the Deleter class and invoking Deleter& operator=(Deleter&)?

Nice.

http://codereview.chromium.org/11149006/diff/14002/base/memory/scoped_ptr.h#n...
base/memory/scoped_ptr.h:229: using std::swap;
I seem to recall (and of course, without citable reference) that this will
prevent searching for swap methods in the global namespace.

I ran into something similar with std::operator<<(std::ostream&) overloads in
printing, and where using ::operator<< would prevent searching when the type in
doubly-nested namespaces (eg: net::foo::SomeMethod() with using std::operator<<
searched within net::foo and std::, but not within net:: or ::)

doubt it's all that important though... maybe.

http://codereview.chromium.org/11149006/diff/14002/base/memory/scoped_ptr.h#n...
base/memory/scoped_ptr.h:242: template <typename U, typename V> friend class
scoped_ptr_impl;
I didn't think MSVC let you get away with this.

http://codereview.chromium.org/11149006/diff/14002/base/memory/scoped_ptr.h#n...
base/memory/scoped_ptr.h:287: explicit scoped_ptr(element_type* p = NULL) :
impl_(p) { }
This is the ONE place that I actually like default arguments. I think having

scoped_ptr() : impl_(NULL) {}
explicit scoped_ptr(element_type* p) : impl_(p) {}

is just... silly.

That said, the style guide says what the style guide says...

http://codereview.chromium.org/11149006/diff/14002/base/memory/scoped_ptr.h#n...
base/memory/scoped_ptr.h:362: scoped_ptr<PassAsType, PasAsDeleter> PassAs() {
Did you accidentally an S? ( Pas*s*AsDeleter )

http://codereview.chromium.org/11149006/diff/14002/base/memory/scoped_ptr.h#n...
base/memory/scoped_ptr.h:392: // There is no way to create an uninitialized
scoped_ptr.
line 392 seems superflous

[awong, 2012-10-18 03:50:21]

2 quick responses before heading to bed.

On Wed, Oct 17, 2012 at 8:17 PM, <rsleevi@chromium.org> wrote:

> Before you work too hard on getting it cleaned up - does it work on
> Windows?
>
> MSVC has been a persistent thorn in my side throughout my career when
> dealing
> with templates, so I'll be surprised if some of this works. I suspect the
> switch
> to MSVC 2010 *may* finally have it doing something decent (MSVC 6/7/8 were
> god-awful with template resolution), but I just want to make sure you're
> iterating concurrently on the right platforms.
>

Most of these tricks are already used in Bind() so I think we're safe but,
yeah, need to check.




> http://codereview.chromium.**org/11149006/diff/14002/base/**
>
memory/scoped_ptr.h<http://codereview.chromium.org/11149006/diff/14002/base/memory/scoped_ptr.h>
> File base/memory/scoped_ptr.h (right):
>
> http://codereview.chromium.**org/11149006/diff/14002/base/**
>
memory/scoped_ptr.h#newcode165<http://codereview.chromium.org/11149006/diff/14002/base/memory/scoped_ptr.h#newcode165>
> base/memory/scoped_ptr.h:165: template <class Element, class Deleter>
> Why not keep the naming consistent with std::, in calling the type T
> instead.
>
> http://codereview.chromium.**org/11149006/diff/14002/base/**
>
memory/scoped_ptr.h#newcode180<http://codereview.chromium.org/11149006/diff/14002/base/memory/scoped_ptr.h#newcode180>
> base/memory/scoped_ptr.h:180: get_deleter() = other.get_deleter();
> So I'm not misreading - you're intentionally relying on type punning
> (shearing? stripping) to ensure that |data_.ptr| is not copied over
> (since it's being explicitly cast to the Deleter class and invoking
> Deleter& operator=(Deleter&)?
>
> Nice.
>

Yeah, this trick and almost all others are courtesy of gromer@.  I take no
credit. :)


>
> http://codereview.chromium.**org/11149006/diff/14002/base/**
>
memory/scoped_ptr.h#newcode229<http://codereview.chromium.org/11149006/diff/14002/base/memory/scoped_ptr.h#newcode229>
> base/memory/scoped_ptr.h:229: using std::swap;
> I seem to recall (and of course, without citable reference) that this
> will prevent searching for swap methods in the global namespace.
>
> I ran into something similar with std::operator<<(std::ostream&)
> overloads in printing, and where using ::operator<< would prevent
> searching when the type in doubly-nested namespaces (eg:
> net::foo::SomeMethod() with using std::operator<< searched within
> net::foo and std::, but not within net:: or ::)
>
> doubt it's all that important though... maybe.
>
> http://codereview.chromium.**org/11149006/diff/14002/base/**
>
memory/scoped_ptr.h#newcode242<http://codereview.chromium.org/11149006/diff/14002/base/memory/scoped_ptr.h#newcode242>
> base/memory/scoped_ptr.h:242: template <typename U, typename V> friend
> class scoped_ptr_impl;
> I didn't think MSVC let you get away with this.
>
> http://codereview.chromium.**org/11149006/diff/14002/base/**
>
memory/scoped_ptr.h#newcode287<http://codereview.chromium.org/11149006/diff/14002/base/memory/scoped_ptr.h#newcode287>
> base/memory/scoped_ptr.h:287: explicit scoped_ptr(element_type* p =
> NULL) : impl_(p) { }
> This is the ONE place that I actually like default arguments. I think
> having
>
> scoped_ptr() : impl_(NULL) {}
> explicit scoped_ptr(element_type* p) : impl_(p) {}
>
> is just... silly.
>
> That said, the style guide says what the style guide says...
>
> http://codereview.chromium.**org/11149006/diff/14002/base/**
>
memory/scoped_ptr.h#newcode362<http://codereview.chromium.org/11149006/diff/14002/base/memory/scoped_ptr.h#newcode362>
> base/memory/scoped_ptr.h:362: scoped_ptr<PassAsType, PasAsDeleter>
> PassAs() {
> Did you accidentally an S? ( Pas*s*AsDeleter )
>
> http://codereview.chromium.**org/11149006/diff/14002/base/**
>
memory/scoped_ptr.h#newcode392<http://codereview.chromium.org/11149006/diff/14002/base/memory/scoped_ptr.h#newcode392>
> base/memory/scoped_ptr.h:392: // There is no way to create an
> uninitialized scoped_ptr.
> line 392 seems superflous
>
>
http://codereview.chromium.**org/11149006/<http://codereview.chromium.org/111...
>

[Jeffrey Yasskin, 2012-10-18 04:14:29]

http://codereview.chromium.org/11149006/diff/14002/base/memory/scoped_ptr.h
File base/memory/scoped_ptr.h (right):

http://codereview.chromium.org/11149006/diff/14002/base/memory/scoped_ptr.h#n...
base/memory/scoped_ptr.h:176: // "move()" function. Do I need to use SFINAE to
make this work?  Or should
I'd like to get a general move() function, which can forward to .Pass() on
move-only types, and be implemented as:
template<typename T> T move(T arg) {
  T result;
  using std::swap;
  swap(arg, result);
  return result;
}
for copyable types. However, I don't know that there's enough interest in
C++98-move efficiency to really justify the function.

For now, I think you can ignore move-only deleters, and assume they're copyable.
A future change can add move support if it becomes important.

http://codereview.chromium.org/11149006/diff/14002/base/memory/scoped_ptr.h#n...
base/memory/scoped_ptr.h:229: using std::swap;
On 2012/10/18 03:17:10, Ryan Sleevi wrote:
> I seem to recall (and of course, without citable reference) that this will
> prevent searching for swap methods in the global namespace.

Yep. swap methods that people want found need to be in the same namespace as the
type they're swapping, not the global namespace. ADL is the only reliable way to
find functions.

> I ran into something similar with std::operator<<(std::ostream&) overloads in
> printing, and where using ::operator<< would prevent searching when the type
in
> doubly-nested namespaces (eg: net::foo::SomeMethod() with using
std::operator<<
> searched within net::foo and std::, but not within net:: or ::)
> 
> doubt it's all that important though... maybe.

http://codereview.chromium.org/11149006/diff/14002/base/memory/scoped_ptr.h#n...
base/memory/scoped_ptr.h:287: explicit scoped_ptr(element_type* p = NULL) :
impl_(p) { }
On 2012/10/18 03:17:10, Ryan Sleevi wrote:
> This is the ONE place that I actually like default arguments. I think having
> 
> scoped_ptr() : impl_(NULL) {}
> explicit scoped_ptr(element_type* p) : impl_(p) {}
> 
> is just... silly.
> 
> That said, the style guide says what the style guide says...

Yeah. Meh on my part.

http://codereview.chromium.org/11149006/diff/14002/base/memory/scoped_ptr.h#n...
base/memory/scoped_ptr.h:296: // TODO(ajwong): REVIEWER QUESTION: is it cleaner
to use the swap() idiom?
The "swap idiom" means to me the operator= technique, which I think is the right
way to implement operator=, but doesn't apply to the move constructor.

You can't actually use swap() in the move constructor unless you first
initialize the object to "empty" before calling swap().

http://codereview.chromium.org/11149006/diff/14002/base/memory/scoped_ptr.h#n...
base/memory/scoped_ptr.h:398: //   inherently unsafe to access an array through
a pointer whose
There's vague wording on subscripting at [expr.add]p5 saying "When an expression
that has integral type is added to or subtracted from a pointer, the result has
the type of the pointer operand. If the pointer operand points to an element of
an array object, and the array is large enough, the result points to an element
offset from the original element such that the difference of the subscripts of
the resulting and original array elements equals the integral expression."

If the pointer has the type of a base class object, it points to a subobject of
an element of an array object, which is not "an element of an array object", so
the if doesn't apply. I've had trouble finding clearer wording forbidding this,
but it obviously won't work.

Deletion is clearer: "In the second alternative (delete array) if the dynamic
type of the object to be deleted differs from its static type, the behavior is
undefined." —[expr.delete]p3

http://codereview.chromium.org/11149006/diff/14002/base/memory/scoped_ptr.h#n...
base/memory/scoped_ptr.h:404: // - it cannot be const-qualified differently from
Element. You can work
Interesting. What goes wrong with this?

[awong, 2012-10-18 18:08:02]

Comments responded to.  Also fixed the safe-bool idiom.

After I get it compiling everywhere, I'll check the disassembly to make sure I
didn't regress perf, then I'll go add unittests and then I'll send it for a
final review.

Feel free to send earlier comments if you want though.

http://codereview.chromium.org/11149006/diff/14002/base/memory/scoped_ptr.h
File base/memory/scoped_ptr.h (right):

http://codereview.chromium.org/11149006/diff/14002/base/memory/scoped_ptr.h#n...
base/memory/scoped_ptr.h:165: template <class Element, class Deleter>
On 2012/10/18 03:17:10, Ryan Sleevi wrote:
> Why not keep the naming consistent with std::, in calling the type T instead.

I don't have a strong preference.

We already have base::DefaultDeleter instead of base::default_deleter which
would better parallel c++11 std::default_deleter so there's precedent to
"Googlify" the style.

http://codereview.chromium.org/11149006/diff/14002/base/memory/scoped_ptr.h#n...
base/memory/scoped_ptr.h:176: // "move()" function. Do I need to use SFINAE to
make this work?  Or should
On 2012/10/18 04:14:29, Jeffrey Yasskin wrote:
> I'd like to get a general move() function, which can forward to .Pass() on
> move-only types, and be implemented as:
> template<typename T> T move(T arg) {
>   T result;
>   using std::swap;
>   swap(arg, result);
>   return result;
> }
> for copyable types. However, I don't know that there's enough interest in
> C++98-move efficiency to really justify the function.
> 
> For now, I think you can ignore move-only deleters, and assume they're
copyable.
> A future change can add move support if it becomes important.

It could be useful in specifically this situation.

The reason I avoided introducing a base::move() or similar is because the C++03
move emulation is incomplete. Specifically, if you write a function that uses
our RValue type to try and be rvalue-reference only, it will also happily accept
lvalues and execute a move on them. Bad.

I think I'll take your advice and leave support out for move-only deleters. Then
we don't need to worry about this.

http://codereview.chromium.org/11149006/diff/14002/base/memory/scoped_ptr.h#n...
base/memory/scoped_ptr.h:180: get_deleter() = other.get_deleter();
On 2012/10/18 03:17:10, Ryan Sleevi wrote:
> So I'm not misreading - you're intentionally relying on type punning
(shearing?
> stripping) to ensure that |data_.ptr| is not copied over (since it's being
> explicitly cast to the Deleter class and invoking Deleter&
operator=(Deleter&)?
> 
> Nice.

Yep. Credit gromer@.

http://codereview.chromium.org/11149006/diff/14002/base/memory/scoped_ptr.h#n...
base/memory/scoped_ptr.h:242: template <typename U, typename V> friend class
scoped_ptr_impl;
On 2012/10/18 03:17:10, Ryan Sleevi wrote:
> I didn't think MSVC let you get away with this.

Okay, will test. Worst case, I just merge scoped_ptr_impl back into both
specializations.

http://codereview.chromium.org/11149006/diff/14002/base/memory/scoped_ptr.h#n...
base/memory/scoped_ptr.h:287: explicit scoped_ptr(element_type* p = NULL) :
impl_(p) { }
On 2012/10/18 04:14:29, Jeffrey Yasskin wrote:
> On 2012/10/18 03:17:10, Ryan Sleevi wrote:
> > This is the ONE place that I actually like default arguments. I think having
> > 
> > scoped_ptr() : impl_(NULL) {}
> > explicit scoped_ptr(element_type* p) : impl_(p) {}
> > 
> > is just... silly.
> > 
> > That said, the style guide says what the style guide says...
> 
> Yeah. Meh on my part.

I guess I'll stick with the style guide. The C++ readability mantle grows heavy.

http://codereview.chromium.org/11149006/diff/14002/base/memory/scoped_ptr.h#n...
base/memory/scoped_ptr.h:296: // TODO(ajwong): REVIEWER QUESTION: is it cleaner
to use the swap() idiom?
On 2012/10/18 04:14:29, Jeffrey Yasskin wrote:
> The "swap idiom" means to me the operator= technique, which I think is the
right
> way to implement operator=, but doesn't apply to the move constructor.
> 
> You can't actually use swap() in the move constructor unless you first
> initialize the object to "empty" before calling swap().

Yes, I was thinking exactly

scoped_ptr(RValue rvalue) : impl_(NULL) { swap(*rvalue.object); }

But your comment makes me thing that you prefer the current implementation?  I'm
honestly completely ambivalent.

http://codereview.chromium.org/11149006/diff/14002/base/memory/scoped_ptr.h#n...
base/memory/scoped_ptr.h:362: scoped_ptr<PassAsType, PasAsDeleter> PassAs() {
On 2012/10/18 03:17:10, Ryan Sleevi wrote:
> Did you accidentally an S? ( Pas*s*AsDeleter )

heh...yes I did. But I also just had to remove deleter conversion support from
PassAs because C++98 can't use default template parameters for function
templates. <grumble />

http://codereview.chromium.org/11149006/diff/14002/base/memory/scoped_ptr.h#n...
base/memory/scoped_ptr.h:392: // There is no way to create an uninitialized
scoped_ptr.
On 2012/10/18 03:17:10, Ryan Sleevi wrote:
> line 392 seems superflous

Yeah. It was just keeping in line with the old impl, but I'll remove it.

http://codereview.chromium.org/11149006/diff/14002/base/memory/scoped_ptr.h#n...
base/memory/scoped_ptr.h:398: //   inherently unsafe to access an array through
a pointer whose
On 2012/10/18 04:14:29, Jeffrey Yasskin wrote:
> There's vague wording on subscripting at [expr.add]p5 saying "When an
expression
> that has integral type is added to or subtracted from a pointer, the result
has
> the type of the pointer operand. If the pointer operand points to an element
of
> an array object, and the array is large enough, the result points to an
element
> offset from the original element such that the difference of the subscripts of
> the resulting and original array elements equals the integral expression."
> 
> If the pointer has the type of a base class object, it points to a subobject
of
> an element of an array object, which is not "an element of an array object",
so
> the if doesn't apply. I've had trouble finding clearer wording forbidding
this,
> but it obviously won't work.
> 
> Deletion is clearer: "In the second alternative (delete array) if the dynamic
> type of the object to be deleted differs from its static type, the behavior is
> undefined." —[expr.delete]p3

Let me ask gromer@ for pointers. This is really his comment.

http://codereview.chromium.org/11149006/diff/14002/base/memory/scoped_ptr.h#n...
base/memory/scoped_ptr.h:404: // - it cannot be const-qualified differently from
Element. You can work
On 2012/10/18 04:14:29, Jeffrey Yasskin wrote:
> Interesting. What goes wrong with this?

Will ask gromer@

[Jeffrey Yasskin, 2012-10-18 18:33:45]

http://codereview.chromium.org/11149006/diff/14002/base/memory/scoped_ptr.h
File base/memory/scoped_ptr.h (right):

http://codereview.chromium.org/11149006/diff/14002/base/memory/scoped_ptr.h#n...
base/memory/scoped_ptr.h:296: // TODO(ajwong): REVIEWER QUESTION: is it cleaner
to use the swap() idiom?
On 2012/10/18 18:08:02, awong wrote:
> On 2012/10/18 04:14:29, Jeffrey Yasskin wrote:
> > The "swap idiom" means to me the operator= technique, which I think is the
> right
> > way to implement operator=, but doesn't apply to the move constructor.
> > 
> > You can't actually use swap() in the move constructor unless you first
> > initialize the object to "empty" before calling swap().
> 
> Yes, I was thinking exactly
> 
> scoped_ptr(RValue rvalue) : impl_(NULL) { swap(*rvalue.object); }
> 
> But your comment makes me thing that you prefer the current implementation? 
I'm
> honestly completely ambivalent. 

I somewhat prefer the current implementation.

[Ryan Sleevi, 2012-10-18 18:39:40]

On 2012/10/18 18:33:45, Jeffrey Yasskin wrote:
> http://codereview.chromium.org/11149006/diff/14002/base/memory/scoped_ptr.h
> File base/memory/scoped_ptr.h (right):
> 
>
http://codereview.chromium.org/11149006/diff/14002/base/memory/scoped_ptr.h#n...
> base/memory/scoped_ptr.h:296: // TODO(ajwong): REVIEWER QUESTION: is it
cleaner
> to use the swap() idiom?
> On 2012/10/18 18:08:02, awong wrote:
> > On 2012/10/18 04:14:29, Jeffrey Yasskin wrote:
> > > The "swap idiom" means to me the operator= technique, which I think is the
> > right
> > > way to implement operator=, but doesn't apply to the move constructor.
> > > 
> > > You can't actually use swap() in the move constructor unless you first
> > > initialize the object to "empty" before calling swap().
> > 
> > Yes, I was thinking exactly
> > 
> > scoped_ptr(RValue rvalue) : impl_(NULL) { swap(*rvalue.object); }
> > 
> > But your comment makes me thing that you prefer the current implementation? 
> I'm
> > honestly completely ambivalent. 
> 
> I somewhat prefer the current implementation.

+1. I don't think the swap/move emulation has been necessary, so I'd rather KISS
than fully emulate unique_ptr

[gromer, 2012-10-18 20:45:20]

I'm not at all familiar with the Chromium codebase, so consider my comments
advisory.

http://codereview.chromium.org/11149006/diff/14002/base/memory/scoped_ptr.h
File base/memory/scoped_ptr.h (right):

http://codereview.chromium.org/11149006/diff/14002/base/memory/scoped_ptr.h#n...
base/memory/scoped_ptr.h:165: template <class Element, class Deleter>
On 2012/10/18 18:08:02, awong wrote:
> On 2012/10/18 03:17:10, Ryan Sleevi wrote:
> > Why not keep the naming consistent with std::, in calling the type T
instead.
> 
> I don't have a strong preference.
> 
> We already have base::DefaultDeleter instead of base::default_deleter which
> would better parallel c++11 std::default_deleter so there's precedent to
> "Googlify" the style.

Note that unique_ptr has element_type and deleter_type typedefs, which in
principle might be added to scoped_ptr; at that point I think the similarity
between these parameter names and the corresponding typedefs would be confusing.

So I'd say stick with single-letter names for the template parameters, and if
you're changing them at all, change them to the names from the standard (T and
D). If you're concerned about readability, add those typedefs and use them
instead of the parameter names in the class body.

http://codereview.chromium.org/11149006/diff/14002/base/memory/scoped_ptr.h#n...
base/memory/scoped_ptr.h:287: explicit scoped_ptr(element_type* p = NULL) :
impl_(p) { }
On 2012/10/18 18:08:02, awong wrote:
> On 2012/10/18 04:14:29, Jeffrey Yasskin wrote:
> > On 2012/10/18 03:17:10, Ryan Sleevi wrote:
> > > This is the ONE place that I actually like default arguments. I think
having
> > > 
> > > scoped_ptr() : impl_(NULL) {}
> > > explicit scoped_ptr(element_type* p) : impl_(p) {}
> > > 
> > > is just... silly.
> > > 
> > > That said, the style guide says what the style guide says...
> > 
> > Yeah. Meh on my part.
> 
> I guess I'll stick with the style guide. The C++ readability mantle grows
heavy.

FWIW the standard specifies the 0- and 1-arg constructors separately, but uses a
default parameter for reset.

http://codereview.chromium.org/11149006/diff/14002/base/memory/scoped_ptr.h#n...
base/memory/scoped_ptr.h:398: //   inherently unsafe to access an array through
a pointer whose
On 2012/10/18 18:08:02, awong wrote:
> On 2012/10/18 04:14:29, Jeffrey Yasskin wrote:
> > There's vague wording on subscripting at [expr.add]p5 saying "When an
> expression
> > that has integral type is added to or subtracted from a pointer, the result
> has
> > the type of the pointer operand. If the pointer operand points to an element
> of
> > an array object, and the array is large enough, the result points to an
> element
> > offset from the original element such that the difference of the subscripts
of
> > the resulting and original array elements equals the integral expression."
> > 
> > If the pointer has the type of a base class object, it points to a subobject
> of
> > an element of an array object, which is not "an element of an array object",
> so
> > the if doesn't apply. I've had trouble finding clearer wording forbidding
> this,
> > but it obviously won't work.

From [basic.compound]p3: "If an object of type T is located at an address A, a
pointer of type cv T* whose value is the
address A is said to point to that object, regardless of how the value was
obtained." So, for example, the 'if' definitely seems to apply if the base and
derived classes are standard-layout. Defect report?

> > 
> > Deletion is clearer: "In the second alternative (delete array) if the
dynamic
> > type of the object to be deleted differs from its static type, the behavior
is
> > undefined." —[expr.delete]p3
> 
> Let me ask gromer@ for pointers. This is really his comment.

The bottom line here is that this is what unique_ptr does, and the goal is to
make scoped_ptr a pure subset of unique_ptr. I inferred that the objective of
this restriction in unique_ptr was to prevent slicing, but I don't have any
specific evidence of that, and some of the standardization history suggests that
the focus was on deletion:

http://cplusplus.github.com/LWG/lwg-defects.html#938

In any event, I'm not sure what you're asking for WRT this comment. I suppose it
would be somewhat more accurate to say "index into" rather than "access"- would
that address your concern? I'd prefer to keep the emphasis on the risk of
slicing rather than the risk of bad deletion, because I don't want people to
think they can evade this issue with a custom deleter.

http://codereview.chromium.org/11149006/diff/14002/base/memory/scoped_ptr.h#n...
base/memory/scoped_ptr.h:404: // - it cannot be const-qualified differently from
Element. You can work
On 2012/10/18 18:08:02, awong wrote:
> On 2012/10/18 04:14:29, Jeffrey Yasskin wrote:
> > Interesting. What goes wrong with this?
> 
> Will ask gromer@

Nothing; it's perfectly safe, but forbidden by unique_ptr, and so we must forbid
it for compatibility. See http://cplusplus.github.com/LWG/lwg-active.html#2118.

I gather Chromium has no near-term prospect of using unique_ptr due to the need
to support older toolchains, so the need to be a strict subset is less pressing.
That being the case, you could always remove this restriction in the Chromium
codebase. In fact, having an actual implementation might be useful data for that
DR, since AFAICT removing this restriction while maintaining the
no-derived-pointer restriction will require nontrivial metaprogramming.

http://codereview.chromium.org/11149006/diff/15009/base/memory/scoped_ptr.h
File base/memory/scoped_ptr.h (right):

http://codereview.chromium.org/11149006/diff/15009/base/memory/scoped_ptr.h#n...
base/memory/scoped_ptr.h:116: struct DefaultDeleter {
I named this in camel-case because it's intended to be an implementation detail,
so there was no need to be consistent with std::default_deleter; I figured
nobody's likely to need to specialize the default deleter before we deprecate
scoped_ptr.

However, as noted in my other comment I gather Chromium's scoped_ptr is here for
the long haul. That being the case, you might consider either moving this to
namespace internal (which come to think of it is what I should've done from the
beginning) or naming it default_deleter, depending on whether you want to expose
it as a customization point.

[willchan no longer on Chromium, 2012-10-18 21:28:41]

http://codereview.chromium.org/11149006/diff/14002/base/memory/scoped_ptr.h
File base/memory/scoped_ptr.h (right):

http://codereview.chromium.org/11149006/diff/14002/base/memory/scoped_ptr.h#n...
base/memory/scoped_ptr.h:404: // - it cannot be const-qualified differently from
Element. You can work
On 2012/10/18 20:45:20, gromer wrote:
> On 2012/10/18 18:08:02, awong wrote:
> > On 2012/10/18 04:14:29, Jeffrey Yasskin wrote:
> > > Interesting. What goes wrong with this?
> > 
> > Will ask gromer@
> 
> Nothing; it's perfectly safe, but forbidden by unique_ptr, and so we must
forbid
> it for compatibility. See
http://cplusplus.github.com/LWG/lwg-active.html#2118.
> 
> I gather Chromium has no near-term prospect of using unique_ptr due to the
need
> to support older toolchains, so the need to be a strict subset is less
pressing.
> That being the case, you could always remove this restriction in the Chromium
> codebase. In fact, having an actual implementation might be useful data for
that
> DR, since AFAICT removing this restriction while maintaining the
> no-derived-pointer restriction will require nontrivial metaprogramming.

We actually do hope to switch to using unique_ptr in the future. It'll be awhile
until all our platforms support it, but it's definitely on the horizon.

[Jeffrey Yasskin, 2012-10-18 22:23:16]

I defer to Geoffrey on this. He's been much closer to the unique_ptr
migration than I have been.

On Thu, Oct 18, 2012 at 1:45 PM,  <gromer@google.com> wrote:
> I'm not at all familiar with the Chromium codebase, so consider my comments
> advisory.
>
>
>
> http://codereview.chromium.org/11149006/diff/14002/base/memory/scoped_ptr.h
> File base/memory/scoped_ptr.h (right):
>
>
http://codereview.chromium.org/11149006/diff/14002/base/memory/scoped_ptr.h#n...
> base/memory/scoped_ptr.h:165: template <class Element, class Deleter>
> On 2012/10/18 18:08:02, awong wrote:
>>
>> On 2012/10/18 03:17:10, Ryan Sleevi wrote:
>> > Why not keep the naming consistent with std::, in calling the type T
>
> instead.
>
>> I don't have a strong preference.
>
>
>> We already have base::DefaultDeleter instead of base::default_deleter
>
> which
>>
>> would better parallel c++11 std::default_deleter so there's precedent
>
> to
>>
>> "Googlify" the style.
>
>
> Note that unique_ptr has element_type and deleter_type typedefs, which
> in principle might be added to scoped_ptr; at that point I think the
> similarity between these parameter names and the corresponding typedefs
> would be confusing.
>
> So I'd say stick with single-letter names for the template parameters,
> and if you're changing them at all, change them to the names from the
> standard (T and D). If you're concerned about readability, add those
> typedefs and use them instead of the parameter names in the class body.
>
>
>
http://codereview.chromium.org/11149006/diff/14002/base/memory/scoped_ptr.h#n...
> base/memory/scoped_ptr.h:287: explicit scoped_ptr(element_type* p =
> NULL) : impl_(p) { }
> On 2012/10/18 18:08:02, awong wrote:
>>
>> On 2012/10/18 04:14:29, Jeffrey Yasskin wrote:
>> > On 2012/10/18 03:17:10, Ryan Sleevi wrote:
>> > > This is the ONE place that I actually like default arguments. I
>
> think having
>>
>> > >
>> > > scoped_ptr() : impl_(NULL) {}
>> > > explicit scoped_ptr(element_type* p) : impl_(p) {}
>> > >
>> > > is just... silly.
>> > >
>> > > That said, the style guide says what the style guide says...
>> >
>> > Yeah. Meh on my part.
>
>
>> I guess I'll stick with the style guide. The C++ readability mantle
>
> grows heavy.
>
> FWIW the standard specifies the 0- and 1-arg constructors separately,
> but uses a default parameter for reset.
>
>
>
http://codereview.chromium.org/11149006/diff/14002/base/memory/scoped_ptr.h#n...
> base/memory/scoped_ptr.h:398: //   inherently unsafe to access an array
> through a pointer whose
> On 2012/10/18 18:08:02, awong wrote:
>>
>> On 2012/10/18 04:14:29, Jeffrey Yasskin wrote:
>> > There's vague wording on subscripting at [expr.add]p5 saying "When
>
> an
>>
>> expression
>> > that has integral type is added to or subtracted from a pointer, the
>
> result
>>
>> has
>> > the type of the pointer operand. If the pointer operand points to an
>
> element
>>
>> of
>> > an array object, and the array is large enough, the result points to
>
> an
>>
>> element
>> > offset from the original element such that the difference of the
>
> subscripts of
>>
>> > the resulting and original array elements equals the integral
>
> expression."
>>
>> >
>> > If the pointer has the type of a base class object, it points to a
>
> subobject
>>
>> of
>> > an element of an array object, which is not "an element of an array
>
> object",
>>
>> so
>> > the if doesn't apply. I've had trouble finding clearer wording
>
> forbidding
>>
>> this,
>> > but it obviously won't work.
>
>
> From [basic.compound]p3: "If an object of type T is located at an
> address A, a pointer of type cv T* whose value is the
> address A is said to point to that object, regardless of how the value
> was obtained." So, for example, the 'if' definitely seems to apply if
> the base and derived classes are standard-layout. Defect report?

Yes! http://www.open-std.org/jtc1/sc22/wg21/docs/cwg_active.html#1504,
which was moved to ready this week:
http://wiki.edg.com/twiki/pub/Wg21portland2012/CoreWorkingGroup/proposed_reso...
(talk to me offline if you need a login)

>> > Deletion is clearer: "In the second alternative (delete array) if
>
> the dynamic
>>
>> > type of the object to be deleted differs from its static type, the
>
> behavior is
>>
>> > undefined." —[expr.delete]p3
>
>
>> Let me ask gromer@ for pointers. This is really his comment.
>
>
> The bottom line here is that this is what unique_ptr does, and the goal
> is to make scoped_ptr a pure subset of unique_ptr. I inferred that the
> objective of this restriction in unique_ptr was to prevent slicing, but
> I don't have any specific evidence of that, and some of the
> standardization history suggests that the focus was on deletion:
>
> http://cplusplus.github.com/LWG/lwg-defects.html#938
>
> In any event, I'm not sure what you're asking for WRT this comment. I
> suppose it would be somewhat more accurate to say "index into" rather
> than "access"- would that address your concern? I'd prefer to keep the
> emphasis on the risk of slicing rather than the risk of bad deletion,
> because I don't want people to think they can evade this issue with a
> custom deleter.

I don't really think the comment needs to change. I was trying to
provide a reference since the comment seemed to be asking for one.

>
http://codereview.chromium.org/11149006/diff/14002/base/memory/scoped_ptr.h#n...
> base/memory/scoped_ptr.h:404: // - it cannot be const-qualified
> differently from Element. You can work
> On 2012/10/18 18:08:02, awong wrote:
>>
>> On 2012/10/18 04:14:29, Jeffrey Yasskin wrote:
>> > Interesting. What goes wrong with this?
>
>
>> Will ask gromer@
>
>
> Nothing; it's perfectly safe, but forbidden by unique_ptr, and so we
> must forbid it for compatibility. See
> http://cplusplus.github.com/LWG/lwg-active.html#2118.

SGTM. I prefer references to defects or the standard over statements
whose reasoning isn't immediately obvious.

> I gather Chromium has no near-term prospect of using unique_ptr due to
> the need to support older toolchains, so the need to be a strict subset
> is less pressing. That being the case, you could always remove this
> restriction in the Chromium codebase. In fact, having an actual
> implementation might be useful data for that DR, since AFAICT removing
> this restriction while maintaining the no-derived-pointer restriction
> will require nontrivial metaprogramming.
>
> http://codereview.chromium.org/11149006/diff/15009/base/memory/scoped_ptr.h
> File base/memory/scoped_ptr.h (right):
>
>
http://codereview.chromium.org/11149006/diff/15009/base/memory/scoped_ptr.h#n...
> base/memory/scoped_ptr.h:116: struct DefaultDeleter {
> I named this in camel-case because it's intended to be an implementation
> detail, so there was no need to be consistent with std::default_deleter;
> I figured nobody's likely to need to specialize the default deleter
> before we deprecate scoped_ptr.
>
> However, as noted in my other comment I gather Chromium's scoped_ptr is
> here for the long haul. That being the case, you might consider either
> moving this to namespace internal (which come to think of it is what I
> should've done from the beginning) or naming it default_deleter,
> depending on whether you want to expose it as a customization point.
>
> http://codereview.chromium.org/11149006/

[Jeffrey Yasskin, 2012-10-18 22:25:24]

Oops, +gromer now.

On Thu, Oct 18, 2012 at 3:22 PM, Jeffrey Yasskin <jyasskin@chromium.org> wrote:
> I defer to Geoffrey on this. He's been much closer to the unique_ptr
> migration than I have been.
>
> On Thu, Oct 18, 2012 at 1:45 PM,  <gromer@google.com> wrote:
>> I'm not at all familiar with the Chromium codebase, so consider my comments
>> advisory.
>>
>>
>>
>> http://codereview.chromium.org/11149006/diff/14002/base/memory/scoped_ptr.h
>> File base/memory/scoped_ptr.h (right):
>>
>>
http://codereview.chromium.org/11149006/diff/14002/base/memory/scoped_ptr.h#n...
>> base/memory/scoped_ptr.h:165: template <class Element, class Deleter>
>> On 2012/10/18 18:08:02, awong wrote:
>>>
>>> On 2012/10/18 03:17:10, Ryan Sleevi wrote:
>>> > Why not keep the naming consistent with std::, in calling the type T
>>
>> instead.
>>
>>> I don't have a strong preference.
>>
>>
>>> We already have base::DefaultDeleter instead of base::default_deleter
>>
>> which
>>>
>>> would better parallel c++11 std::default_deleter so there's precedent
>>
>> to
>>>
>>> "Googlify" the style.
>>
>>
>> Note that unique_ptr has element_type and deleter_type typedefs, which
>> in principle might be added to scoped_ptr; at that point I think the
>> similarity between these parameter names and the corresponding typedefs
>> would be confusing.
>>
>> So I'd say stick with single-letter names for the template parameters,
>> and if you're changing them at all, change them to the names from the
>> standard (T and D). If you're concerned about readability, add those
>> typedefs and use them instead of the parameter names in the class body.
>>
>>
>>
http://codereview.chromium.org/11149006/diff/14002/base/memory/scoped_ptr.h#n...
>> base/memory/scoped_ptr.h:287: explicit scoped_ptr(element_type* p =
>> NULL) : impl_(p) { }
>> On 2012/10/18 18:08:02, awong wrote:
>>>
>>> On 2012/10/18 04:14:29, Jeffrey Yasskin wrote:
>>> > On 2012/10/18 03:17:10, Ryan Sleevi wrote:
>>> > > This is the ONE place that I actually like default arguments. I
>>
>> think having
>>>
>>> > >
>>> > > scoped_ptr() : impl_(NULL) {}
>>> > > explicit scoped_ptr(element_type* p) : impl_(p) {}
>>> > >
>>> > > is just... silly.
>>> > >
>>> > > That said, the style guide says what the style guide says...
>>> >
>>> > Yeah. Meh on my part.
>>
>>
>>> I guess I'll stick with the style guide. The C++ readability mantle
>>
>> grows heavy.
>>
>> FWIW the standard specifies the 0- and 1-arg constructors separately,
>> but uses a default parameter for reset.
>>
>>
>>
http://codereview.chromium.org/11149006/diff/14002/base/memory/scoped_ptr.h#n...
>> base/memory/scoped_ptr.h:398: //   inherently unsafe to access an array
>> through a pointer whose
>> On 2012/10/18 18:08:02, awong wrote:
>>>
>>> On 2012/10/18 04:14:29, Jeffrey Yasskin wrote:
>>> > There's vague wording on subscripting at [expr.add]p5 saying "When
>>
>> an
>>>
>>> expression
>>> > that has integral type is added to or subtracted from a pointer, the
>>
>> result
>>>
>>> has
>>> > the type of the pointer operand. If the pointer operand points to an
>>
>> element
>>>
>>> of
>>> > an array object, and the array is large enough, the result points to
>>
>> an
>>>
>>> element
>>> > offset from the original element such that the difference of the
>>
>> subscripts of
>>>
>>> > the resulting and original array elements equals the integral
>>
>> expression."
>>>
>>> >
>>> > If the pointer has the type of a base class object, it points to a
>>
>> subobject
>>>
>>> of
>>> > an element of an array object, which is not "an element of an array
>>
>> object",
>>>
>>> so
>>> > the if doesn't apply. I've had trouble finding clearer wording
>>
>> forbidding
>>>
>>> this,
>>> > but it obviously won't work.
>>
>>
>> From [basic.compound]p3: "If an object of type T is located at an
>> address A, a pointer of type cv T* whose value is the
>> address A is said to point to that object, regardless of how the value
>> was obtained." So, for example, the 'if' definitely seems to apply if
>> the base and derived classes are standard-layout. Defect report?
>
> Yes! http://www.open-std.org/jtc1/sc22/wg21/docs/cwg_active.html#1504,
> which was moved to ready this week:
>
http://wiki.edg.com/twiki/pub/Wg21portland2012/CoreWorkingGroup/proposed_reso...
> (talk to me offline if you need a login)
>
>>> > Deletion is clearer: "In the second alternative (delete array) if
>>
>> the dynamic
>>>
>>> > type of the object to be deleted differs from its static type, the
>>
>> behavior is
>>>
>>> > undefined." —[expr.delete]p3
>>
>>
>>> Let me ask gromer@ for pointers. This is really his comment.
>>
>>
>> The bottom line here is that this is what unique_ptr does, and the goal
>> is to make scoped_ptr a pure subset of unique_ptr. I inferred that the
>> objective of this restriction in unique_ptr was to prevent slicing, but
>> I don't have any specific evidence of that, and some of the
>> standardization history suggests that the focus was on deletion:
>>
>> http://cplusplus.github.com/LWG/lwg-defects.html#938
>>
>> In any event, I'm not sure what you're asking for WRT this comment. I
>> suppose it would be somewhat more accurate to say "index into" rather
>> than "access"- would that address your concern? I'd prefer to keep the
>> emphasis on the risk of slicing rather than the risk of bad deletion,
>> because I don't want people to think they can evade this issue with a
>> custom deleter.
>
> I don't really think the comment needs to change. I was trying to
> provide a reference since the comment seemed to be asking for one.
>
>>
http://codereview.chromium.org/11149006/diff/14002/base/memory/scoped_ptr.h#n...
>> base/memory/scoped_ptr.h:404: // - it cannot be const-qualified
>> differently from Element. You can work
>> On 2012/10/18 18:08:02, awong wrote:
>>>
>>> On 2012/10/18 04:14:29, Jeffrey Yasskin wrote:
>>> > Interesting. What goes wrong with this?
>>
>>
>>> Will ask gromer@
>>
>>
>> Nothing; it's perfectly safe, but forbidden by unique_ptr, and so we
>> must forbid it for compatibility. See
>> http://cplusplus.github.com/LWG/lwg-active.html#2118.
>
> SGTM. I prefer references to defects or the standard over statements
> whose reasoning isn't immediately obvious.
>
>> I gather Chromium has no near-term prospect of using unique_ptr due to
>> the need to support older toolchains, so the need to be a strict subset
>> is less pressing. That being the case, you could always remove this
>> restriction in the Chromium codebase. In fact, having an actual
>> implementation might be useful data for that DR, since AFAICT removing
>> this restriction while maintaining the no-derived-pointer restriction
>> will require nontrivial metaprogramming.
>>
>> http://codereview.chromium.org/11149006/diff/15009/base/memory/scoped_ptr.h
>> File base/memory/scoped_ptr.h (right):
>>
>>
http://codereview.chromium.org/11149006/diff/15009/base/memory/scoped_ptr.h#n...
>> base/memory/scoped_ptr.h:116: struct DefaultDeleter {
>> I named this in camel-case because it's intended to be an implementation
>> detail, so there was no need to be consistent with std::default_deleter;
>> I figured nobody's likely to need to specialize the default deleter
>> before we deprecate scoped_ptr.
>>
>> However, as noted in my other comment I gather Chromium's scoped_ptr is
>> here for the long haul. That being the case, you might consider either
>> moving this to namespace internal (which come to think of it is what I
>> should've done from the beginning) or naming it default_deleter,
>> depending on whether you want to expose it as a customization point.
>>
>> http://codereview.chromium.org/11149006/

[tfarina, 2012-11-02 02:12:31]

http://codereview.chromium.org/11149006/diff/23004/base/memory/scoped_ptr.h
File base/memory/scoped_ptr.h (right):

http://codereview.chromium.org/11149006/diff/23004/base/memory/scoped_ptr.h#n...
base/memory/scoped_ptr.h:281: // Constructor.  Defaults to initializing with
NULL.
can you remove this comment all together while you are touching this code? it's
useless and redundant.

http://codereview.chromium.org/11149006/diff/23004/base/memory/scoped_ptr.h#n...
base/memory/scoped_ptr.h:284: // Constructor.  Takes ownership of p.
can you remove the 'Constructor' part of this comment? We all know that it's a
ctor. If we don't know we shouldn't be touching this code.

[awong, 2012-11-02 02:46:51]

Hi Thiago,

Thanks for the comments.  The code is not really ready for review yet, but
I will note that part of the reason for the weird commenting is that it
parallels the Google internal implementation. That might be of questionable
benefit, but unless there's a strong reason to change, I'd rather keep the
two the same.  It makes porting modifications (like I'm doing now) a little
easier.

-Albert


On Thu, Nov 1, 2012 at 7:12 PM, <tfarina@chromium.org> wrote:

>
> http://codereview.chromium.**org/11149006/diff/23004/base/**
>
memory/scoped_ptr.h<http://codereview.chromium.org/11149006/diff/23004/base/memory/scoped_ptr.h>
> File base/memory/scoped_ptr.h (right):
>
> http://codereview.chromium.**org/11149006/diff/23004/base/**
>
memory/scoped_ptr.h#newcode281<http://codereview.chromium.org/11149006/diff/23004/base/memory/scoped_ptr.h#newcode281>
> base/memory/scoped_ptr.h:281: // Constructor.  Defaults to initializing
> with NULL.
> can you remove this comment all together while you are touching this
> code? it's useless and redundant.
>
> http://codereview.chromium.**org/11149006/diff/23004/base/**
>
memory/scoped_ptr.h#newcode284<http://codereview.chromium.org/11149006/diff/23004/base/memory/scoped_ptr.h#newcode284>
> base/memory/scoped_ptr.h:284: // Constructor.  Takes ownership of p.
> can you remove the 'Constructor' part of this comment? We all know that
> it's a ctor. If we don't know we shouldn't be touching this code.
>
>
http://codereview.chromium.**org/11149006/<http://codereview.chromium.org/111...
>

[awong, 2012-11-27 22:36:15]

Finally wrote tests and scrubbed behavior (fixed some bugs too :-/)

Jeffrey, Geoffrey, Ryan, do you three mind doing a thorough scrubdown of this? 
Though I'm pretty certain it's correct, it's a complex enough change that I am
nervous about it.

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

https://codereview.chromium.org/11149006/diff/14002/base/memory/scoped_ptr.h#...
base/memory/scoped_ptr.h:165: template <class Element, class Deleter>
On 2012/10/18 20:45:20, gromer wrote:
> On 2012/10/18 18:08:02, awong wrote:
> > On 2012/10/18 03:17:10, Ryan Sleevi wrote:
> > > Why not keep the naming consistent with std::, in calling the type T
> instead.
> > 
> > I don't have a strong preference.
> > 
> > We already have base::DefaultDeleter instead of base::default_deleter which
> > would better parallel c++11 std::default_deleter so there's precedent to
> > "Googlify" the style.
> 
> Note that unique_ptr has element_type and deleter_type typedefs, which in
> principle might be added to scoped_ptr; at that point I think the similarity
> between these parameter names and the corresponding typedefs would be
confusing.
> 
> So I'd say stick with single-letter names for the template parameters, and if
> you're changing them at all, change them to the names from the standard (T and
> D). If you're concerned about readability, add those typedefs and use them
> instead of the parameter names in the class body.

Went back to T and D.

https://codereview.chromium.org/11149006/diff/14002/base/memory/scoped_ptr.h#...
base/memory/scoped_ptr.h:287: explicit scoped_ptr(element_type* p = NULL) :
impl_(p) { }
On 2012/10/18 20:45:20, gromer wrote:
> On 2012/10/18 18:08:02, awong wrote:
> > On 2012/10/18 04:14:29, Jeffrey Yasskin wrote:
> > > On 2012/10/18 03:17:10, Ryan Sleevi wrote:
> > > > This is the ONE place that I actually like default arguments. I think
> having
> > > > 
> > > > scoped_ptr() : impl_(NULL) {}
> > > > explicit scoped_ptr(element_type* p) : impl_(p) {}
> > > > 
> > > > is just... silly.
> > > > 
> > > > That said, the style guide says what the style guide says...
> > > 
> > > Yeah. Meh on my part.
> > 
> > I guess I'll stick with the style guide. The C++ readability mantle grows
> heavy.
> 
> FWIW the standard specifies the 0- and 1-arg constructors separately, but uses
a
> default parameter for reset.

All the more reason to avoid default arguments :-/

https://codereview.chromium.org/11149006/diff/14002/base/memory/scoped_ptr.h#...
base/memory/scoped_ptr.h:296: // TODO(ajwong): REVIEWER QUESTION: is it cleaner
to use the swap() idiom?
On 2012/10/18 18:33:45, Jeffrey Yasskin wrote:
> On 2012/10/18 18:08:02, awong wrote:
> > On 2012/10/18 04:14:29, Jeffrey Yasskin wrote:
> > > The "swap idiom" means to me the operator= technique, which I think is the
> > right
> > > way to implement operator=, but doesn't apply to the move constructor.
> > > 
> > > You can't actually use swap() in the move constructor unless you first
> > > initialize the object to "empty" before calling swap().
> > 
> > Yes, I was thinking exactly
> > 
> > scoped_ptr(RValue rvalue) : impl_(NULL) { swap(*rvalue.object); }
> > 
> > But your comment makes me thing that you prefer the current implementation? 
> I'm
> > honestly completely ambivalent. 
> 
> I somewhat prefer the current implementation.

Done.

https://codereview.chromium.org/11149006/diff/14002/base/memory/scoped_ptr.h#...
base/memory/scoped_ptr.h:398: //   inherently unsafe to access an array through
a pointer whose
On 2012/10/18 20:45:20, gromer wrote:
> On 2012/10/18 18:08:02, awong wrote:
> > On 2012/10/18 04:14:29, Jeffrey Yasskin wrote:
> > > There's vague wording on subscripting at [expr.add]p5 saying "When an
> > expression
> > > that has integral type is added to or subtracted from a pointer, the
result
> > has
> > > the type of the pointer operand. If the pointer operand points to an
element
> > of
> > > an array object, and the array is large enough, the result points to an
> > element
> > > offset from the original element such that the difference of the
subscripts
> of
> > > the resulting and original array elements equals the integral expression."
> > > 
> > > If the pointer has the type of a base class object, it points to a
subobject
> > of
> > > an element of an array object, which is not "an element of an array
object",
> > so
> > > the if doesn't apply. I've had trouble finding clearer wording forbidding
> > this,
> > > but it obviously won't work.
> 
> From [basic.compound]p3: "If an object of type T is located at an address A, a
> pointer of type cv T* whose value is the
> address A is said to point to that object, regardless of how the value was
> obtained." So, for example, the 'if' definitely seems to apply if the base and
> derived classes are standard-layout. Defect report?

I don't think I understand this quite enough to write a defect report.
jyasskin@, gromer@, do one of you want to write that up?


> > > 
> > > Deletion is clearer: "In the second alternative (delete array) if the
> dynamic
> > > type of the object to be deleted differs from its static type, the
behavior
> is
> > > undefined." —[expr.delete]p3
> > 
> > Let me ask gromer@ for pointers. This is really his comment.
> 
> The bottom line here is that this is what unique_ptr does, and the goal is to
> make scoped_ptr a pure subset of unique_ptr. I inferred that the objective of
> this restriction in unique_ptr was to prevent slicing, but I don't have any
> specific evidence of that, and some of the standardization history suggests
that
> the focus was on deletion:
> 
> http://cplusplus.github.com/LWG/lwg-defects.html#938
> 
> In any event, I'm not sure what you're asking for WRT this comment. I suppose
it
> would be somewhat more accurate to say "index into" rather than "access"-
would
> that address your concern? I'd prefer to keep the emphasis on the risk of
> slicing rather than the risk of bad deletion, because I don't want people to
> think they can evade this issue with a custom deleter.

The citation is exactly what I was looking for :)  Thanks!

https://codereview.chromium.org/11149006/diff/14002/base/memory/scoped_ptr.h#...
base/memory/scoped_ptr.h:404: // - it cannot be const-qualified differently from
Element. You can work
On 2012/10/18 21:28:42, willchan wrote:
> On 2012/10/18 20:45:20, gromer wrote:
> > On 2012/10/18 18:08:02, awong wrote:
> > > On 2012/10/18 04:14:29, Jeffrey Yasskin wrote:
> > > > Interesting. What goes wrong with this?
> > > 
> > > Will ask gromer@
> > 
> > Nothing; it's perfectly safe, but forbidden by unique_ptr, and so we must
> forbid
> > it for compatibility. See
> http://cplusplus.github.com/LWG/lwg-active.html#2118.
> > 
> > I gather Chromium has no near-term prospect of using unique_ptr due to the
> need
> > to support older toolchains, so the need to be a strict subset is less
> pressing.
> > That being the case, you could always remove this restriction in the
Chromium
> > codebase. In fact, having an actual implementation might be useful data for
> that
> > DR, since AFAICT removing this restriction while maintaining the
> > no-derived-pointer restriction will require nontrivial metaprogramming.
> 
> We actually do hope to switch to using unique_ptr in the future. It'll be
awhile
> until all our platforms support it, but it's definitely on the horizon.

I added the citation, and removed the reference to implicit_cast<> since
chromium doesn't have it.

[awong, 2012-11-28 01:22:41]

FYI, the current implementation causes a ~1% increase in text-segment size on
base_unittests so the performance impact needs to be considered.

I haven't had time to dig into what's causing this.  This was compared using
clang on linux on x86_64.

-Albert

On 2012/11/27 22:36:15, awong wrote:
> Finally wrote tests and scrubbed behavior (fixed some bugs too :-/)
> 
> Jeffrey, Geoffrey, Ryan, do you three mind doing a thorough scrubdown of this?

> Though I'm pretty certain it's correct, it's a complex enough change that I am
> nervous about it.
> 
> https://codereview.chromium.org/11149006/diff/14002/base/memory/scoped_ptr.h
> File base/memory/scoped_ptr.h (right):
> 
>
https://codereview.chromium.org/11149006/diff/14002/base/memory/scoped_ptr.h#...
> base/memory/scoped_ptr.h:165: template <class Element, class Deleter>
> On 2012/10/18 20:45:20, gromer wrote:
> > On 2012/10/18 18:08:02, awong wrote:
> > > On 2012/10/18 03:17:10, Ryan Sleevi wrote:
> > > > Why not keep the naming consistent with std::, in calling the type T
> > instead.
> > > 
> > > I don't have a strong preference.
> > > 
> > > We already have base::DefaultDeleter instead of base::default_deleter
which
> > > would better parallel c++11 std::default_deleter so there's precedent to
> > > "Googlify" the style.
> > 
> > Note that unique_ptr has element_type and deleter_type typedefs, which in
> > principle might be added to scoped_ptr; at that point I think the similarity
> > between these parameter names and the corresponding typedefs would be
> confusing.
> > 
> > So I'd say stick with single-letter names for the template parameters, and
if
> > you're changing them at all, change them to the names from the standard (T
and
> > D). If you're concerned about readability, add those typedefs and use them
> > instead of the parameter names in the class body.
> 
> Went back to T and D.
> 
>
https://codereview.chromium.org/11149006/diff/14002/base/memory/scoped_ptr.h#...
> base/memory/scoped_ptr.h:287: explicit scoped_ptr(element_type* p = NULL) :
> impl_(p) { }
> On 2012/10/18 20:45:20, gromer wrote:
> > On 2012/10/18 18:08:02, awong wrote:
> > > On 2012/10/18 04:14:29, Jeffrey Yasskin wrote:
> > > > On 2012/10/18 03:17:10, Ryan Sleevi wrote:
> > > > > This is the ONE place that I actually like default arguments. I think
> > having
> > > > > 
> > > > > scoped_ptr() : impl_(NULL) {}
> > > > > explicit scoped_ptr(element_type* p) : impl_(p) {}
> > > > > 
> > > > > is just... silly.
> > > > > 
> > > > > That said, the style guide says what the style guide says...
> > > > 
> > > > Yeah. Meh on my part.
> > > 
> > > I guess I'll stick with the style guide. The C++ readability mantle grows
> > heavy.
> > 
> > FWIW the standard specifies the 0- and 1-arg constructors separately, but
uses
> a
> > default parameter for reset.
> 
> All the more reason to avoid default arguments :-/
> 
>
https://codereview.chromium.org/11149006/diff/14002/base/memory/scoped_ptr.h#...
> base/memory/scoped_ptr.h:296: // TODO(ajwong): REVIEWER QUESTION: is it
cleaner
> to use the swap() idiom?
> On 2012/10/18 18:33:45, Jeffrey Yasskin wrote:
> > On 2012/10/18 18:08:02, awong wrote:
> > > On 2012/10/18 04:14:29, Jeffrey Yasskin wrote:
> > > > The "swap idiom" means to me the operator= technique, which I think is
the
> > > right
> > > > way to implement operator=, but doesn't apply to the move constructor.
> > > > 
> > > > You can't actually use swap() in the move constructor unless you first
> > > > initialize the object to "empty" before calling swap().
> > > 
> > > Yes, I was thinking exactly
> > > 
> > > scoped_ptr(RValue rvalue) : impl_(NULL) { swap(*rvalue.object); }
> > > 
> > > But your comment makes me thing that you prefer the current
implementation? 
> > I'm
> > > honestly completely ambivalent. 
> > 
> > I somewhat prefer the current implementation.
> 
> Done.
> 
>
https://codereview.chromium.org/11149006/diff/14002/base/memory/scoped_ptr.h#...
> base/memory/scoped_ptr.h:398: //   inherently unsafe to access an array
through
> a pointer whose
> On 2012/10/18 20:45:20, gromer wrote:
> > On 2012/10/18 18:08:02, awong wrote:
> > > On 2012/10/18 04:14:29, Jeffrey Yasskin wrote:
> > > > There's vague wording on subscripting at [expr.add]p5 saying "When an
> > > expression
> > > > that has integral type is added to or subtracted from a pointer, the
> result
> > > has
> > > > the type of the pointer operand. If the pointer operand points to an
> element
> > > of
> > > > an array object, and the array is large enough, the result points to an
> > > element
> > > > offset from the original element such that the difference of the
> subscripts
> > of
> > > > the resulting and original array elements equals the integral
expression."
> > > > 
> > > > If the pointer has the type of a base class object, it points to a
> subobject
> > > of
> > > > an element of an array object, which is not "an element of an array
> object",
> > > so
> > > > the if doesn't apply. I've had trouble finding clearer wording
forbidding
> > > this,
> > > > but it obviously won't work.
> > 
> > From [basic.compound]p3: "If an object of type T is located at an address A,
a
> > pointer of type cv T* whose value is the
> > address A is said to point to that object, regardless of how the value was
> > obtained." So, for example, the 'if' definitely seems to apply if the base
and
> > derived classes are standard-layout. Defect report?
> 
> I don't think I understand this quite enough to write a defect report.
> jyasskin@, gromer@, do one of you want to write that up?
> 
> 
> > > > 
> > > > Deletion is clearer: "In the second alternative (delete array) if the
> > dynamic
> > > > type of the object to be deleted differs from its static type, the
> behavior
> > is
> > > > undefined." —[expr.delete]p3
> > > 
> > > Let me ask gromer@ for pointers. This is really his comment.
> > 
> > The bottom line here is that this is what unique_ptr does, and the goal is
to
> > make scoped_ptr a pure subset of unique_ptr. I inferred that the objective
of
> > this restriction in unique_ptr was to prevent slicing, but I don't have any
> > specific evidence of that, and some of the standardization history suggests
> that
> > the focus was on deletion:
> > 
> > http://cplusplus.github.com/LWG/lwg-defects.html#938
> > 
> > In any event, I'm not sure what you're asking for WRT this comment. I
suppose
> it
> > would be somewhat more accurate to say "index into" rather than "access"-
> would
> > that address your concern? I'd prefer to keep the emphasis on the risk of
> > slicing rather than the risk of bad deletion, because I don't want people to
> > think they can evade this issue with a custom deleter.
> 
> The citation is exactly what I was looking for :)  Thanks!
> 
>
https://codereview.chromium.org/11149006/diff/14002/base/memory/scoped_ptr.h#...
> base/memory/scoped_ptr.h:404: // - it cannot be const-qualified differently
from
> Element. You can work
> On 2012/10/18 21:28:42, willchan wrote:
> > On 2012/10/18 20:45:20, gromer wrote:
> > > On 2012/10/18 18:08:02, awong wrote:
> > > > On 2012/10/18 04:14:29, Jeffrey Yasskin wrote:
> > > > > Interesting. What goes wrong with this?
> > > > 
> > > > Will ask gromer@
> > > 
> > > Nothing; it's perfectly safe, but forbidden by unique_ptr, and so we must
> > forbid
> > > it for compatibility. See
> > http://cplusplus.github.com/LWG/lwg-active.html#2118.
> > > 
> > > I gather Chromium has no near-term prospect of using unique_ptr due to the
> > need
> > > to support older toolchains, so the need to be a strict subset is less
> > pressing.
> > > That being the case, you could always remove this restriction in the
> Chromium
> > > codebase. In fact, having an actual implementation might be useful data
for
> > that
> > > DR, since AFAICT removing this restriction while maintaining the
> > > no-derived-pointer restriction will require nontrivial metaprogramming.
> > 
> > We actually do hope to switch to using unique_ptr in the future. It'll be
> awhile
> > until all our platforms support it, but it's definitely on the horizon.
> 
> I added the citation, and removed the reference to implicit_cast<> since
> chromium doesn't have it.

[Jeffrey Yasskin, 2012-11-28 06:07:18]

I'm curious to hear what caused the 1% size increase. Unless it's just the extra
code in scoped_ptr_unittest—that would be boring.

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

https://codereview.chromium.org/11149006/diff/47012/base/memory/scoped_ptr.h#...
base/memory/scoped_ptr.h:146: //     static_cast<int>(malloc(sizeof(int))));
static_cast<int*>, right?

https://codereview.chromium.org/11149006/diff/47012/base/memory/scoped_ptr.h#...
base/memory/scoped_ptr.h:148: inline void operator()(void* ptr) const {
This isn't going to work on const or volatile pointers, but maybe we don't care
much, especially about the volatile ones.

https://codereview.chromium.org/11149006/diff/47012/base/memory/scoped_ptr.h#...
base/memory/scoped_ptr.h:507: // scoped_array<C> is like scoped_ptr<C>, except
that the caller must allocate
Should this be deprecated in favor of scoped_ptr<T[]>? Is there now any
difference between  scoped_array<C> and scoped_ptr<C[]>?

https://codereview.chromium.org/11149006/diff/47012/base/memory/scoped_ptr.h#...
base/memory/scoped_ptr.h:553: if (array_ != NULL) {
Note that delete[] does nothing if array_ is NULL, so you don't need to add this
line. (There's a corner case when the delete[] is provided by a class, but I
don't think we need to worry about that.)

https://codereview.chromium.org/11149006/diff/47012/base/memory/scoped_ptr.h#...
base/memory/scoped_ptr.h:628: // scoped_ptr_malloc<> is similar to scoped_ptr<>,
but it accepts a
Should this be deprecated in favor of scoped_ptr<T, base::FreeDeleter>?

https://codereview.chromium.org/11149006/diff/47012/content/browser/speech/sp...
File content/browser/speech/speech_recognition_manager_impl.h (right):

https://codereview.chromium.org/11149006/diff/47012/content/browser/speech/sp...
content/browser/speech/speech_recognition_manager_impl.h:93: // Needed for dtor.
This should be 1 line down.

[awong, 2012-11-28 10:20:34]

Comments addressed.

As for the size increase, I examined the assembly more.  Here's some knowns:
  1) in the .o files (probably elided in final link) we generate exported
      instantiations of all templates.  Thus, the DefaultDeleter<> and the
      scoped_ptr_impl<> classes get code generated for them in addition
      to the scoped_ptr<> even though in most call sites, the get inlined.
  2) The inlining has changed
  3) Less things are inlined than I expected. Expl: scoped_ptr::get() isn't
inlined in
      either the old or the new version of scoped_ptr<>.

Here are what I think the current problems are (50% confidence at veracity of
these statements):
  1) Splitting out the default constructor rather than using default argument of
NULL
      causes 2 constructors to be codegen-ed.
  2) Relying on scoped_ptr_impl<>::Pass() causes inefficient codegen in some
places.
      In particular, the templated constructor for scoped_ptr<> is hugely worse
causing
      a tangle of temporaries.
  3) All these changes are causing the optimizer to just do different things.
For example,
      ecx is spilled in the TEST_F for the old implementation, but not in the
new one
      which shifts bunches of things around.

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

https://codereview.chromium.org/11149006/diff/47012/base/memory/scoped_ptr.h#...
base/memory/scoped_ptr.h:146: //     static_cast<int>(malloc(sizeof(int))));
On 2012/11/28 06:07:18, Jeffrey Yasskin wrote:
> static_cast<int*>, right?

Right. :)

https://codereview.chromium.org/11149006/diff/47012/base/memory/scoped_ptr.h#...
base/memory/scoped_ptr.h:148: inline void operator()(void* ptr) const {
On 2012/11/28 06:07:18, Jeffrey Yasskin wrote:
> This isn't going to work on const or volatile pointers, but maybe we don't
care
> much, especially about the volatile ones.

I don't think we care. This is basically staging a replacement of
scoped_ptr_malloc which already has this problem.

https://codereview.chromium.org/11149006/diff/47012/base/memory/scoped_ptr.h#...
base/memory/scoped_ptr.h:507: // scoped_array<C> is like scoped_ptr<C>, except
that the caller must allocate
On 2012/11/28 06:07:18, Jeffrey Yasskin wrote:
> Should this be deprecated in favor of scoped_ptr<T[]>? Is there now any
> difference between  scoped_array<C> and scoped_ptr<C[]>?

Yep. scoped_array and scoped_ptr_malloc should be deprecated. I was refraining
from listing a deprecation notice until I was more confident that this CL
worked, but you're right. That's silly. Noticed added.

https://codereview.chromium.org/11149006/diff/47012/base/memory/scoped_ptr.h#...
base/memory/scoped_ptr.h:553: if (array_ != NULL) {
On 2012/11/28 06:07:18, Jeffrey Yasskin wrote:
> Note that delete[] does nothing if array_ is NULL, so you don't need to add
this
> line. (There's a corner case when the delete[] is provided by a class, but I
> don't think we need to worry about that.)

yeah. Good point. I spent about an hour of headache with an tagentially related
unittest  because scoped_ptr_malloc,>::reset() was calling the deleter even when
it was NULL (this is disallowed by unique_ptr's spec) but you're right that this
case is always benign.

https://codereview.chromium.org/11149006/diff/47012/base/memory/scoped_ptr.h#...
base/memory/scoped_ptr.h:628: // scoped_ptr_malloc<> is similar to scoped_ptr<>,
but it accepts a
On 2012/11/28 06:07:18, Jeffrey Yasskin wrote:
> Should this be deprecated in favor of scoped_ptr<T, base::FreeDeleter>?

Yes.

https://codereview.chromium.org/11149006/diff/47012/content/browser/speech/sp...
File content/browser/speech/speech_recognition_manager_impl.h (right):

https://codereview.chromium.org/11149006/diff/47012/content/browser/speech/sp...
content/browser/speech/speech_recognition_manager_impl.h:93: // Needed for dtor.
On 2012/11/28 06:07:18, Jeffrey Yasskin wrote:
> This should be 1 line down.

Done.

[awong, 2012-11-28 11:01:26]

New results:
  1) Using a separate default constructor versus a default NULL arg has 
      no effect. (I just misread assembly dump)
  2) Stuffing the deleters into an anonymous namespace reduces the code
      size, though actual "delete ptr" call is still not getting inlined.
  3) Using get_deleter()(data_.ptr) instead of data_(data_.ptr) introduced
      another callq to invoke the get_deleter().
  4) Removing the Pass() in the scoped_ptr_impl<> logic reduced the number
      of temporaries. 

It really feels like the compiler should be smarter than this...



On 2012/11/28 10:20:34, awong wrote:
> Comments addressed.
> 
> As for the size increase, I examined the assembly more.  Here's some knowns:
>   1) in the .o files (probably elided in final link) we generate exported
>       instantiations of all templates.  Thus, the DefaultDeleter<> and the
>       scoped_ptr_impl<> classes get code generated for them in addition
>       to the scoped_ptr<> even though in most call sites, the get inlined.
>   2) The inlining has changed
>   3) Less things are inlined than I expected. Expl: scoped_ptr::get() isn't
> inlined in
>       either the old or the new version of scoped_ptr<>.
> 
> Here are what I think the current problems are (50% confidence at veracity of
> these statements):
>   1) Splitting out the default constructor rather than using default argument
of
> NULL
>       causes 2 constructors to be codegen-ed.
>   2) Relying on scoped_ptr_impl<>::Pass() causes inefficient codegen in some
> places.
>       In particular, the templated constructor for scoped_ptr<> is hugely
worse
> causing
>       a tangle of temporaries.
>   3) All these changes are causing the optimizer to just do different things.
> For example,
>       ecx is spilled in the TEST_F for the old implementation, but not in the
> new one
>       which shifts bunches of things around.
> 
> https://codereview.chromium.org/11149006/diff/47012/base/memory/scoped_ptr.h
> File base/memory/scoped_ptr.h (right):
> 
>
https://codereview.chromium.org/11149006/diff/47012/base/memory/scoped_ptr.h#...
> base/memory/scoped_ptr.h:146: //     static_cast<int>(malloc(sizeof(int))));
> On 2012/11/28 06:07:18, Jeffrey Yasskin wrote:
> > static_cast<int*>, right?
> 
> Right. :)
> 
>
https://codereview.chromium.org/11149006/diff/47012/base/memory/scoped_ptr.h#...
> base/memory/scoped_ptr.h:148: inline void operator()(void* ptr) const {
> On 2012/11/28 06:07:18, Jeffrey Yasskin wrote:
> > This isn't going to work on const or volatile pointers, but maybe we don't
> care
> > much, especially about the volatile ones.
> 
> I don't think we care. This is basically staging a replacement of
> scoped_ptr_malloc which already has this problem.
> 
>
https://codereview.chromium.org/11149006/diff/47012/base/memory/scoped_ptr.h#...
> base/memory/scoped_ptr.h:507: // scoped_array<C> is like scoped_ptr<C>, except
> that the caller must allocate
> On 2012/11/28 06:07:18, Jeffrey Yasskin wrote:
> > Should this be deprecated in favor of scoped_ptr<T[]>? Is there now any
> > difference between  scoped_array<C> and scoped_ptr<C[]>?
> 
> Yep. scoped_array and scoped_ptr_malloc should be deprecated. I was refraining
> from listing a deprecation notice until I was more confident that this CL
> worked, but you're right. That's silly. Noticed added.
> 
>
https://codereview.chromium.org/11149006/diff/47012/base/memory/scoped_ptr.h#...
> base/memory/scoped_ptr.h:553: if (array_ != NULL) {
> On 2012/11/28 06:07:18, Jeffrey Yasskin wrote:
> > Note that delete[] does nothing if array_ is NULL, so you don't need to add
> this
> > line. (There's a corner case when the delete[] is provided by a class, but I
> > don't think we need to worry about that.)
> 
> yeah. Good point. I spent about an hour of headache with an tagentially
related
> unittest  because scoped_ptr_malloc,>::reset() was calling the deleter even
when
> it was NULL (this is disallowed by unique_ptr's spec) but you're right that
this
> case is always benign.
> 
>
https://codereview.chromium.org/11149006/diff/47012/base/memory/scoped_ptr.h#...
> base/memory/scoped_ptr.h:628: // scoped_ptr_malloc<> is similar to
scoped_ptr<>,
> but it accepts a
> On 2012/11/28 06:07:18, Jeffrey Yasskin wrote:
> > Should this be deprecated in favor of scoped_ptr<T, base::FreeDeleter>?
> 
> Yes.
> 
>
https://codereview.chromium.org/11149006/diff/47012/content/browser/speech/sp...
> File content/browser/speech/speech_recognition_manager_impl.h (right):
> 
>
https://codereview.chromium.org/11149006/diff/47012/content/browser/speech/sp...
> content/browser/speech/speech_recognition_manager_impl.h:93: // Needed for
dtor.
> On 2012/11/28 06:07:18, Jeffrey Yasskin wrote:
> > This should be 1 line down.
> 
> Done.

[awong, 2012-11-28 11:38:00]

!@#$#@#  Oh good grief.  Mystery solved.

I've been accidentally compiling -O0.  Good call Jeffrey on that earlier. I
thought I double checked it, but I was wrong.  In -O2, the generated code is
identical. The object file's text segement is still larger on account of some
clang generated pretty-print functions, but I don't think that matters as much.

A silver lining though is this debugging work isn't quite wasted. Because people
do build in debug mode quite often, I think it's still worth making
scoped_ptr_impl<> not use the move-emulation trick.  It was just a niceness and
it causes crazy temporary blowup.

A more debatable change is to go back to gromer@'s
static_cast<D&>(data_)(data_.ptr) syntax for invoking the deleter rather than
calling get_deleter()(data_.ptr).  Yes it's grosser, but again, unnecessarily
wasteful of a function call.

New version uploaded.

On 2012/11/28 11:01:26, awong wrote:
> New results:
>   1) Using a separate default constructor versus a default NULL arg has 
>       no effect. (I just misread assembly dump)
>   2) Stuffing the deleters into an anonymous namespace reduces the code
>       size, though actual "delete ptr" call is still not getting inlined.
>   3) Using get_deleter()(data_.ptr) instead of data_(data_.ptr) introduced
>       another callq to invoke the get_deleter().
>   4) Removing the Pass() in the scoped_ptr_impl<> logic reduced the number
>       of temporaries. 
> 
> It really feels like the compiler should be smarter than this...
> 
> 
> 
> On 2012/11/28 10:20:34, awong wrote:
> > Comments addressed.
> > 
> > As for the size increase, I examined the assembly more.  Here's some knowns:
> >   1) in the .o files (probably elided in final link) we generate exported
> >       instantiations of all templates.  Thus, the DefaultDeleter<> and the
> >       scoped_ptr_impl<> classes get code generated for them in addition
> >       to the scoped_ptr<> even though in most call sites, the get inlined.
> >   2) The inlining has changed
> >   3) Less things are inlined than I expected. Expl: scoped_ptr::get() isn't
> > inlined in
> >       either the old or the new version of scoped_ptr<>.
> > 
> > Here are what I think the current problems are (50% confidence at veracity
of
> > these statements):
> >   1) Splitting out the default constructor rather than using default
argument
> of
> > NULL
> >       causes 2 constructors to be codegen-ed.
> >   2) Relying on scoped_ptr_impl<>::Pass() causes inefficient codegen in some
> > places.
> >       In particular, the templated constructor for scoped_ptr<> is hugely
> worse
> > causing
> >       a tangle of temporaries.
> >   3) All these changes are causing the optimizer to just do different
things.
> > For example,
> >       ecx is spilled in the TEST_F for the old implementation, but not in
the
> > new one
> >       which shifts bunches of things around.
> > 
> > https://codereview.chromium.org/11149006/diff/47012/base/memory/scoped_ptr.h
> > File base/memory/scoped_ptr.h (right):
> > 
> >
>
https://codereview.chromium.org/11149006/diff/47012/base/memory/scoped_ptr.h#...
> > base/memory/scoped_ptr.h:146: //     static_cast<int>(malloc(sizeof(int))));
> > On 2012/11/28 06:07:18, Jeffrey Yasskin wrote:
> > > static_cast<int*>, right?
> > 
> > Right. :)
> > 
> >
>
https://codereview.chromium.org/11149006/diff/47012/base/memory/scoped_ptr.h#...
> > base/memory/scoped_ptr.h:148: inline void operator()(void* ptr) const {
> > On 2012/11/28 06:07:18, Jeffrey Yasskin wrote:
> > > This isn't going to work on const or volatile pointers, but maybe we don't
> > care
> > > much, especially about the volatile ones.
> > 
> > I don't think we care. This is basically staging a replacement of
> > scoped_ptr_malloc which already has this problem.
> > 
> >
>
https://codereview.chromium.org/11149006/diff/47012/base/memory/scoped_ptr.h#...
> > base/memory/scoped_ptr.h:507: // scoped_array<C> is like scoped_ptr<C>,
except
> > that the caller must allocate
> > On 2012/11/28 06:07:18, Jeffrey Yasskin wrote:
> > > Should this be deprecated in favor of scoped_ptr<T[]>? Is there now any
> > > difference between  scoped_array<C> and scoped_ptr<C[]>?
> > 
> > Yep. scoped_array and scoped_ptr_malloc should be deprecated. I was
refraining
> > from listing a deprecation notice until I was more confident that this CL
> > worked, but you're right. That's silly. Noticed added.
> > 
> >
>
https://codereview.chromium.org/11149006/diff/47012/base/memory/scoped_ptr.h#...
> > base/memory/scoped_ptr.h:553: if (array_ != NULL) {
> > On 2012/11/28 06:07:18, Jeffrey Yasskin wrote:
> > > Note that delete[] does nothing if array_ is NULL, so you don't need to
add
> > this
> > > line. (There's a corner case when the delete[] is provided by a class, but
I
> > > don't think we need to worry about that.)
> > 
> > yeah. Good point. I spent about an hour of headache with an tagentially
> related
> > unittest  because scoped_ptr_malloc,>::reset() was calling the deleter even
> when
> > it was NULL (this is disallowed by unique_ptr's spec) but you're right that
> this
> > case is always benign.
> > 
> >
>
https://codereview.chromium.org/11149006/diff/47012/base/memory/scoped_ptr.h#...
> > base/memory/scoped_ptr.h:628: // scoped_ptr_malloc<> is similar to
> scoped_ptr<>,
> > but it accepts a
> > On 2012/11/28 06:07:18, Jeffrey Yasskin wrote:
> > > Should this be deprecated in favor of scoped_ptr<T, base::FreeDeleter>?
> > 
> > Yes.
> > 
> >
>
https://codereview.chromium.org/11149006/diff/47012/content/browser/speech/sp...
> > File content/browser/speech/speech_recognition_manager_impl.h (right):
> > 
> >
>
https://codereview.chromium.org/11149006/diff/47012/content/browser/speech/sp...
> > content/browser/speech/speech_recognition_manager_impl.h:93: // Needed for
> dtor.
> > On 2012/11/28 06:07:18, Jeffrey Yasskin wrote:
> > > This should be 1 line down.
> > 
> > Done.

[Jeffrey Yasskin, 2012-11-28 22:06:42]

LGTM

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

https://codereview.chromium.org/11149006/diff/40024/base/memory/scoped_ptr.h#...
base/memory/scoped_ptr.h:176: scoped_ptr_impl(scoped_ptr_impl<U, V>* other)
Could you DISALLOW_COPY_AND_ASSIGN so there's no way to accidentally miss
calling this constructor?

https://codereview.chromium.org/11149006/diff/40024/base/memory/scoped_ptr.h#...
base/memory/scoped_ptr.h:402: // - it cannot be const-qualified differently from
T per unique_ptr spec.
Perhaps mention that users should implicit_cast<const T*> to avoid this
restriction, but they should *not* implicit_cast<Base*> to get around the first
bullet.

[awong, 2012-11-29 01:15:13]

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

https://codereview.chromium.org/11149006/diff/40024/base/memory/scoped_ptr.h#...
base/memory/scoped_ptr.h:176: scoped_ptr_impl(scoped_ptr_impl<U, V>* other)
On 2012/11/28 22:06:42, Jeffrey Yasskin wrote:
> Could you DISALLOW_COPY_AND_ASSIGN so there's no way to accidentally miss
> calling this constructor?

Done.

https://codereview.chromium.org/11149006/diff/40024/base/memory/scoped_ptr.h#...
base/memory/scoped_ptr.h:402: // - it cannot be const-qualified differently from
T per unique_ptr spec.
On 2012/11/28 22:06:42, Jeffrey Yasskin wrote:
> Perhaps mention that users should implicit_cast<const T*> to avoid this
> restriction, but they should *not* implicit_cast<Base*> to get around the
first
> bullet.

Done and also beefed up the first bullet.

[Ryan Sleevi, 2012-11-29 01:42:25]

Initial comments. I've probably missed a few things, so may go through it again
once I've thought about other things for a bit.

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

https://codereview.chromium.org/11149006/diff/40024/base/memory/scoped_ptr.h#...
base/memory/scoped_ptr.h:119: // All default single-object deleters can
trivially convert to one another.
20.7.1.1.2 2 has

"This constructor shall not participate in overload resolution unless U* is
implicitly convertable to T*."

I don't see anything in this class participating in that distinction.

I realize scoped_ptr_impl is probably guaranteeing this, by virtue of the
implicit conversion in scoped_ptr_impl::reset/scoped_ptr_impl::swap, so it's
probably a non-issue, but worth noting.

It seems like we can use an enable_if trick to ensure is_convertable, and then
let SFINAE remove this from the overload set if T/U are incomplete or
non-convertable types.

https://codereview.chromium.org/11149006/diff/40024/base/memory/scoped_ptr.h#...
base/memory/scoped_ptr.h:129: struct DefaultDeleter<T[]> {
You're missing the constructors here - in particular, the type conversion
handler.

https://codereview.chromium.org/11149006/diff/40024/base/memory/scoped_ptr.h#...
base/memory/scoped_ptr.h:222: D& get_deleter() { return data_; }
Shouldn't you remove and then re-add the reference for D, in the event that D
was specified as a reference/const-reference? (unique_ptr permits this for
deleters)

https://codereview.chromium.org/11149006/diff/40024/base/memory/scoped_ptr.h#...
base/memory/scoped_ptr.h:270: template <class T, class D =
base::DefaultDeleter<T> >
20.7.1.2.1 requires that D be a function object type, lvalue-reference to
function, or lvalue-reference to a function object type.

Meaning, in particular, that D should not be a pointer type, AIUI. This is
enforced in libcxx, not sure about libstdc++.

https://codereview.chromium.org/11149006/diff/40024/base/memory/scoped_ptr.h#...
base/memory/scoped_ptr.h:278: // The element and deleter types.
20.7.1.2.3 defines the ::pointer type, as according to

remove_reference<D>::type::pointer (if it exists), otherwise it's a synonym for
T*. This is due to the allocator_traits<> requirements (20.7.1.2.4)

Probably a non-issue, given that we don't use allocator_traits<> directly in
Chromium code, but worth mentioning about spec divergence.

https://codereview.chromium.org/11149006/diff/40024/base/memory/scoped_ptr.h#...
base/memory/scoped_ptr.h:294: scoped_ptr(scoped_ptr<U, V> other) :
impl_(&other.impl_) { }
Note 20.7.1.2.1.19, which discards the move constructor from overload resolution
unless:
- D is a reference type and E (V) is the same type as D, or D is not a reference
type and E (V) is implicitly convertable to D.


In particular, and unless I'm missing something, this seems to allow you to have
a scoped_ptr<A[]> be released into a scoped_ptr<B>, assuming A and B are
convertable - which would be bad, right?

If not, why specialize scoped_ptr<A[]> at all (line 380), and not just instead
rely on the specialization of DefaultDeleter (line 129)?

https://codereview.chromium.org/11149006/diff/40024/base/memory/scoped_ptr.h#...
base/memory/scoped_ptr.h:302: scoped_ptr& operator=(scoped_ptr<U, V> rhs) {
same concerns here raised on 294

This is called out in 20.7.1.2.3.5, which requires that U not be an array type.

https://codereview.chromium.org/11149006/diff/40024/base/memory/scoped_ptr.h#...
base/memory/scoped_ptr.h:421: assert(impl_.get() != NULL);
I didn't think we "liked" using assert() - or at least, security team I seem to
recall had some sort of beef with it and ASAN.

I may be misremembering.

[Ryan Sleevi, 2012-11-29 02:42:27]

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

https://codereview.chromium.org/11149006/diff/44004/base/memory/scoped_ptr.h#...
base/memory/scoped_ptr.h:170: class scoped_ptr_impl {
Have you confirmed what the compile time implications are of having this common
template interface?

I realize the desire to favour readability, but it seems like you're forcing
potentially four template instantiations during resolution. That is, for any
given T, it needs to see scoped_ptr<T,D>, which requires instantiation
scoped_ptr_impl<T,D>, then see if the specialization of scoped_ptr<T[],D> fits,
which requires instantiation of scoped_ptr_impl<T[],D>. At least, AIUI the
current resolution processes in clang and MSVC.

It seems that not having a common base, as admittedly harmful as that may be to
readability, and just having the two overloads (T and T[]), combined with the
two deleters (D<V> and D<V[]>), may make a slight-in-isolation-but-large-overall
perf impact.

May be micro-optimization, but given that tricks like SFINAE-ing the Pass()
implementation for Callback() caused a 3% MSVC regression, or the extra arg for
base::Bind was something like a 16% regression, I want to make sure we're not
going too crazy.

[gromer, 2012-12-04 19:41:18]

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

https://codereview.chromium.org/11149006/diff/40024/base/memory/scoped_ptr.h#...
base/memory/scoped_ptr.h:119: // All default single-object deleters can
trivially convert to one another.
On 2012/11/29 01:42:26, Ryan Sleevi wrote:
> 20.7.1.1.2 2 has
> 
> "This constructor shall not participate in overload resolution unless U* is
> implicitly convertable to T*."
> 
> I don't see anything in this class participating in that distinction.
> 
> I realize scoped_ptr_impl is probably guaranteeing this, by virtue of the
> implicit conversion in scoped_ptr_impl::reset/scoped_ptr_impl::swap, so it's
> probably a non-issue, but worth noting.
> 
> It seems like we can use an enable_if trick to ensure is_convertable, and then
> let SFINAE remove this from the overload set if T/U are incomplete or
> non-convertable types.

Oh, I see, this is an issue only because of the Chromium scoped_ptr's support
for move-emulation, so it doesn't apply to the Google-internal one. ajwong, this
one's in your court. FWIW I don't think SFINAE tricks are called for here, but a
comment might be appropriate.

More generally, it might make sense to explicitly document that DefaultDeleter
is an implementation detail of scoped_ptr, and should not be explicitly used by
client code.

https://codereview.chromium.org/11149006/diff/40024/base/memory/scoped_ptr.h#...
base/memory/scoped_ptr.h:129: struct DefaultDeleter<T[]> {
On 2012/11/29 01:42:26, Ryan Sleevi wrote:
> You're missing the constructors here - in particular, the type conversion
> handler.

"Missing" relative to what? std::default_delete<T[]> is specified to have only
one constructor, the 0-arg constructor, which is specified to use the default
implementation. That's exactly equivalent to what's provided here.

https://codereview.chromium.org/11149006/diff/40024/base/memory/scoped_ptr.h#...
base/memory/scoped_ptr.h:294: scoped_ptr(scoped_ptr<U, V> other) :
impl_(&other.impl_) { }
On 2012/11/29 01:42:26, Ryan Sleevi wrote:
> Note 20.7.1.2.1.19, which discards the move constructor from overload
resolution
> unless:
> - D is a reference type and E (V) is the same type as D, or D is not a
reference
> type and E (V) is implicitly convertable to D.
> 
> 
> In particular, and unless I'm missing something, this seems to allow you to
have
> a scoped_ptr<A[]> be released into a scoped_ptr<B>, assuming A and B are
> convertable - which would be bad, right?
> 

As noted, the simulated move semantics are outside my expertise, but FWIW this
concern sounds plausible to me.

> If not, why specialize scoped_ptr<A[]> at all (line 380), and not just instead
> rely on the specialization of DefaultDeleter (line 129)?

scoped_ptr<A[]> has a different method set than the base template; in
particular, it lacks operator* and operator->, and provides operator[] instead.

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

https://codereview.chromium.org/11149006/diff/44004/base/memory/scoped_ptr.h#...
base/memory/scoped_ptr.h:217: // on the current sequencing.
Maybe add a tracking bug for this too?

https://codereview.chromium.org/11149006/diff/44004/base/memory/scoped_ptr.h#...
base/memory/scoped_ptr.h:335: private:
This violates the style guide by interleaving private with public, and by
interleaving type declarations with function declarations. Is there any other
way?

[Ryan Sleevi, 2012-12-04 20:09:51]

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

https://codereview.chromium.org/11149006/diff/40024/base/memory/scoped_ptr.h#...
base/memory/scoped_ptr.h:119: // All default single-object deleters can
trivially convert to one another.
On 2012/12/04 19:41:18, gromer wrote:
> On 2012/11/29 01:42:26, Ryan Sleevi wrote:
> > 20.7.1.1.2 2 has
> > 
> > "This constructor shall not participate in overload resolution unless U* is
> > implicitly convertable to T*."
> > 
> > I don't see anything in this class participating in that distinction.
> > 
> > I realize scoped_ptr_impl is probably guaranteeing this, by virtue of the
> > implicit conversion in scoped_ptr_impl::reset/scoped_ptr_impl::swap, so it's
> > probably a non-issue, but worth noting.
> > 
> > It seems like we can use an enable_if trick to ensure is_convertable, and
then
> > let SFINAE remove this from the overload set if T/U are incomplete or
> > non-convertable types.
> 
> Oh, I see, this is an issue only because of the Chromium scoped_ptr's support
> for move-emulation, so it doesn't apply to the Google-internal one. ajwong,
this
> one's in your court. FWIW I don't think SFINAE tricks are called for here, but
a
> comment might be appropriate.
> 
> More generally, it might make sense to explicitly document that DefaultDeleter
> is an implementation detail of scoped_ptr, and should not be explicitly used
by
> client code.

gromer: I'm not sure I follow your remark. The spec comment is about the
constructor, but as implemented, the implicit type conversion isn't tested until
invoking operator()(T*), and finding that the U* originally used (for
DefaultDeleter<U>) doesn't allow conversion to T* - hence, operator()(U*) is
unresolved.

I agree that it's "mostly the same", but the note about 20.7.1.1.2.1 seems
applicable.

https://codereview.chromium.org/11149006/diff/40024/base/memory/scoped_ptr.h#...
base/memory/scoped_ptr.h:129: struct DefaultDeleter<T[]> {
On 2012/12/04 19:41:18, gromer wrote:
> On 2012/11/29 01:42:26, Ryan Sleevi wrote:
> > You're missing the constructors here - in particular, the type conversion
> > handler.
> 
> "Missing" relative to what? std::default_delete<T[]> is specified to have only
> one constructor, the 0-arg constructor, which is specified to use the default
> implementation. That's exactly equivalent to what's provided here.

Sorry, ignore this. It's a disconnect between libc++, libstdc++, and MSVC (aka
dinkumware). I'd go with the spec here (20.7.1.1.3)

[gromer, 2012-12-04 20:39:26]

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

https://codereview.chromium.org/11149006/diff/40024/base/memory/scoped_ptr.h#...
base/memory/scoped_ptr.h:119: // All default single-object deleters can
trivially convert to one another.
On 2012/12/04 20:09:51, Ryan Sleevi wrote:
> On 2012/12/04 19:41:18, gromer wrote:
> > On 2012/11/29 01:42:26, Ryan Sleevi wrote:
> > > 20.7.1.1.2 2 has
> > > 
> > > "This constructor shall not participate in overload resolution unless U*
is
> > > implicitly convertable to T*."
> > > 
> > > I don't see anything in this class participating in that distinction.
> > > 
> > > I realize scoped_ptr_impl is probably guaranteeing this, by virtue of the
> > > implicit conversion in scoped_ptr_impl::reset/scoped_ptr_impl::swap, so
it's
> > > probably a non-issue, but worth noting.
> > > 
> > > It seems like we can use an enable_if trick to ensure is_convertable, and
> then
> > > let SFINAE remove this from the overload set if T/U are incomplete or
> > > non-convertable types.
> > 
> > Oh, I see, this is an issue only because of the Chromium scoped_ptr's
support
> > for move-emulation, so it doesn't apply to the Google-internal one. ajwong,
> this
> > one's in your court. FWIW I don't think SFINAE tricks are called for here,
but
> a
> > comment might be appropriate.
> > 
> > More generally, it might make sense to explicitly document that
DefaultDeleter
> > is an implementation detail of scoped_ptr, and should not be explicitly used
> by
> > client code.
> 
> gromer: I'm not sure I follow your remark. The spec comment is about the
> constructor, but as implemented, the implicit type conversion isn't tested
until
> invoking operator()(T*), and finding that the U* originally used (for
> DefaultDeleter<U>) doesn't allow conversion to T* - hence, operator()(U*) is
> unresolved.
> 
> I agree that it's "mostly the same", but the note about 20.7.1.1.2.1 seems
> applicable.

I'm afraid I don't follow you either; I was mostly agreeing with you that this
is "probably a non-issue, but worth noting". Let me try to rephrase:

First of all, this isn't really my department since it's not an issue for the
scoped_ptr that I maintain, so my comments should be considered advisory at
best.

That being said, my hunch is that it would be difficult to write client code
where the presence or absence of this SFINAE trick makes a visible difference,
and all but impossible to do so without making explicit mention of
DefaultDeleter. I'm open to refutation; what would be especially helpful is
example client code that is affected by the issue.

That being the case, my recommendation would to add a comment to discourage
independent use of DefaultDeleter, but not worry about the extra mess that
SFINAE tricks would entail. Remember, this is DefaultDeleter, not
std::default_delete; we only need to worry about inconsistency with the standard
to the extent that the difference can lead to actual bugs, and/or make it harder
to port scoped_ptr clients to unique_ptr in the future

[awong, 2012-12-12 02:17:02]

Ready for another pass.

Overall, I opted to punt on most of the issues raised following the "Gromer
subset rule". ;P

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

https://codereview.chromium.org/11149006/diff/40024/base/memory/scoped_ptr.h#...
base/memory/scoped_ptr.h:119: // All default single-object deleters can
trivially convert to one another.
On 2012/12/04 20:39:27, gromer wrote:
> On 2012/12/04 20:09:51, Ryan Sleevi wrote:
> > On 2012/12/04 19:41:18, gromer wrote:
> > > On 2012/11/29 01:42:26, Ryan Sleevi wrote:
> > > > 20.7.1.1.2 2 has
> > > > 
> > > > "This constructor shall not participate in overload resolution unless U*
> is
> > > > implicitly convertable to T*."
> > > > 
> > > > I don't see anything in this class participating in that distinction.
> > > > 
> > > > I realize scoped_ptr_impl is probably guaranteeing this, by virtue of
the
> > > > implicit conversion in scoped_ptr_impl::reset/scoped_ptr_impl::swap, so
> it's
> > > > probably a non-issue, but worth noting.
> > > > 
> > > > It seems like we can use an enable_if trick to ensure is_convertable,
and
> > then
> > > > let SFINAE remove this from the overload set if T/U are incomplete or
> > > > non-convertable types.
> > > 
> > > Oh, I see, this is an issue only because of the Chromium scoped_ptr's
> support
> > > for move-emulation, so it doesn't apply to the Google-internal one.
ajwong,
> > this
> > > one's in your court. FWIW I don't think SFINAE tricks are called for here,
> but
> > a
> > > comment might be appropriate.
> > > 
> > > More generally, it might make sense to explicitly document that
> DefaultDeleter
> > > is an implementation detail of scoped_ptr, and should not be explicitly
used
> > by
> > > client code.
> > 
> > gromer: I'm not sure I follow your remark. The spec comment is about the
> > constructor, but as implemented, the implicit type conversion isn't tested
> until
> > invoking operator()(T*), and finding that the U* originally used (for
> > DefaultDeleter<U>) doesn't allow conversion to T* - hence, operator()(U*) is
> > unresolved.
> > 
> > I agree that it's "mostly the same", but the note about 20.7.1.1.2.1 seems
> > applicable.
> 
> I'm afraid I don't follow you either; I was mostly agreeing with you that this
> is "probably a non-issue, but worth noting". Let me try to rephrase:
> 
> First of all, this isn't really my department since it's not an issue for the
> scoped_ptr that I maintain, so my comments should be considered advisory at
> best.
> 
> That being said, my hunch is that it would be difficult to write client code
> where the presence or absence of this SFINAE trick makes a visible difference,
> and all but impossible to do so without making explicit mention of
> DefaultDeleter. I'm open to refutation; what would be especially helpful is
> example client code that is affected by the issue.
> 
> That being the case, my recommendation would to add a comment to discourage
> independent use of DefaultDeleter, but not worry about the extra mess that
> SFINAE tricks would entail. Remember, this is DefaultDeleter, not
> std::default_delete; we only need to worry about inconsistency with the
standard
> to the extent that the difference can lead to actual bugs, and/or make it
harder
> to port scoped_ptr clients to unique_ptr in the future

Reading through this, I think adding a COMPILE_ASSERT on on the result of
implicit_cast which should force the right typing constraints.

https://codereview.chromium.org/11149006/diff/40024/base/memory/scoped_ptr.h#...
base/memory/scoped_ptr.h:222: D& get_deleter() { return data_; }
On 2012/11/29 01:42:26, Ryan Sleevi wrote:
> Shouldn't you remove and then re-add the reference for D, in the event that D
> was specified as a reference/const-reference? (unique_ptr permits this for
> deleters)

If I'm correct, right now we would just fail if D is a references type correct?

In that case, we're fulfilling the "strict unique_ptr<> subset" goal that
gromer@ was talking about. I'm okay with that since I don't think we have a
usecase needing a reference deleter type and fixing it requires making the
scoped_ptr(T*, d) constructor much more complicated.

https://codereview.chromium.org/11149006/diff/40024/base/memory/scoped_ptr.h#...
base/memory/scoped_ptr.h:270: template <class T, class D =
base::DefaultDeleter<T> >
On 2012/11/29 01:42:26, Ryan Sleevi wrote:
> 20.7.1.2.1 requires that D be a function object type, lvalue-reference to
> function, or lvalue-reference to a function object type.
> 
> Meaning, in particular, that D should not be a pointer type, AIUI. This is
> enforced in libcxx, not sure about libstdc++.

Right now, I think what happens is we don't support function pointers (and I'm
expilcitly ignoreing D& types).

We could support functions pointers by doing something like:

template <class T>
class scoped_ptr<T, void(T*)> { ... do something special ... };

but I want to hold off on this until we have a use case.

https://codereview.chromium.org/11149006/diff/40024/base/memory/scoped_ptr.h#...
base/memory/scoped_ptr.h:278: // The element and deleter types.
On 2012/11/29 01:42:26, Ryan Sleevi wrote:
> 20.7.1.2.3 defines the ::pointer type, as according to
> 
> remove_reference<D>::type::pointer (if it exists), otherwise it's a synonym
for
> T*. This is due to the allocator_traits<> requirements (20.7.1.2.4)
> 
> Probably a non-issue, given that we don't use allocator_traits<> directly in
> Chromium code, but worth mentioning about spec divergence.

Good to know. Going to skip this one as well because it requires
remove_reference<> and I feel weird doing it partially here, but not for
Deleter.

https://codereview.chromium.org/11149006/diff/40024/base/memory/scoped_ptr.h#...
base/memory/scoped_ptr.h:294: scoped_ptr(scoped_ptr<U, V> other) :
impl_(&other.impl_) { }
On 2012/12/04 19:41:18, gromer wrote:
> On 2012/11/29 01:42:26, Ryan Sleevi wrote:
> > Note 20.7.1.2.1.19, which discards the move constructor from overload
> resolution
> > unless:
> > - D is a reference type and E (V) is the same type as D, or D is not a
> reference
> > type and E (V) is implicitly convertable to D.
> > 
> > 
> > In particular, and unless I'm missing something, this seems to allow you to
> have
> > a scoped_ptr<A[]> be released into a scoped_ptr<B>, assuming A and B are
> > convertable - which would be bad, right?
> > 
> 
> As noted, the simulated move semantics are outside my expertise, but FWIW this
> concern sounds plausible to me.
> 
> > If not, why specialize scoped_ptr<A[]> at all (line 380), and not just
instead
> > rely on the specialization of DefaultDeleter (line 129)?
> 
> scoped_ptr<A[]> has a different method set than the base template; in
> particular, it lacks operator* and operator->, and provides operator[]
instead.

Ick...nice catch.

Reading through 20.7.1.2.1.{15..17} and 20.7.1.2.1.{18..21} for
unique_ptr(unique_ptr&& u) and template <typename U, E>
unique_ptr(unique_ptr<U,E> && u), I think the distinction between the two
constructors only exist if

  (a) U is an array type
  (b) D is a reference type

If U is an array type, it shouldn't participate which will result in a compile
failure.  In our impl, a COMPILE_ASSERT should give equivalent safety.

If we disallow D being a reference type, then by spec we technically should
resolve to unique_ptr(unique_ptr&& u).  However, the only behavioral difference
is in the handling of D being a reference so I think we're okay not creating a
second constructor.

Does that sound right to peoples?

BTW, just tested passing from scoped_ptr<T[]> to scoped_ptr<T*>. Apparently we
were inadvertently saved by scoped_ptr<T[]> not friending the impl class.  Still
putting in the COMPILE_ASSERTs just to be explicit.

https://codereview.chromium.org/11149006/diff/40024/base/memory/scoped_ptr.h#...
base/memory/scoped_ptr.h:302: scoped_ptr& operator=(scoped_ptr<U, V> rhs) {
On 2012/11/29 01:42:26, Ryan Sleevi wrote:
> same concerns here raised on 294
> 
> This is called out in 20.7.1.2.3.5, which requires that U not be an array
type.

See previous.

https://codereview.chromium.org/11149006/diff/40024/base/memory/scoped_ptr.h#...
base/memory/scoped_ptr.h:421: assert(impl_.get() != NULL);
On 2012/11/29 01:42:26, Ryan Sleevi wrote:
> I didn't think we "liked" using assert() - or at least, security team I seem
to
> recall had some sort of beef with it and ASAN.
> 
> I may be misremembering.

I waffled on this one, but am leaving it as assert().  Rationale is all the
previous code in this file is also using assert().

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

https://codereview.chromium.org/11149006/diff/44004/base/memory/scoped_ptr.h#...
base/memory/scoped_ptr.h:170: class scoped_ptr_impl {
On 2012/11/29 02:42:27, Ryan Sleevi wrote:
> Have you confirmed what the compile time implications are of having this
common
> template interface?
> 
> I realize the desire to favour readability, but it seems like you're forcing
> potentially four template instantiations during resolution. That is, for any
> given T, it needs to see scoped_ptr<T,D>, which requires instantiation
> scoped_ptr_impl<T,D>, then see if the specialization of scoped_ptr<T[],D>
fits,
> which requires instantiation of scoped_ptr_impl<T[],D>. At least, AIUI the
> current resolution processes in clang and MSVC.
> 
> It seems that not having a common base, as admittedly harmful as that may be
to
> readability, and just having the two overloads (T and T[]), combined with the
> two deleters (D<V> and D<V[]>), may make a
slight-in-isolation-but-large-overall
> perf impact.
> 
> May be micro-optimization, but given that tricks like SFINAE-ing the Pass()
> implementation for Callback() caused a 3% MSVC regression, or the extra arg
for
> base::Bind was something like a 16% regression, I want to make sure we're not
> going too crazy.

I'm not quite sure how to go about testing this.  Do you just run time on winja?

My gut tells me this will not nearly be as expensive. Bind() is a bit degenerate
because of the N^2 template specializations. However, that's gut feel without
data.

https://codereview.chromium.org/11149006/diff/44004/base/memory/scoped_ptr.h#...
base/memory/scoped_ptr.h:217: // on the current sequencing.
On 2012/12/04 19:41:18, gromer wrote:
> Maybe add a tracking bug for this too?

I think http://crbug.com/162971 cited earlier covers this, or are you seeing
something else?

https://codereview.chromium.org/11149006/diff/44004/base/memory/scoped_ptr.h#...
base/memory/scoped_ptr.h:335: private:
On 2012/12/04 19:41:18, gromer wrote:
> This violates the style guide by interleaving private with public, and by
> interleaving type declarations with function declarations. Is there any other
> way?

Yeah...I could move the typedef into a private section, but it makes the idiom
harder to read IMO. Will ask a few other readability reviewers on chromium for
their opinion.

[Ryan Sleevi, 2012-12-12 03:37:46]

I'll do a proper read through when my mind is working. I just quickly skimmed
your replies and the diffs.

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

https://codereview.chromium.org/11149006/diff/40024/base/memory/scoped_ptr.h#...
base/memory/scoped_ptr.h:222: D& get_deleter() { return data_; }
On 2012/12/12 02:17:03, awong wrote:
> On 2012/11/29 01:42:26, Ryan Sleevi wrote:
> > Shouldn't you remove and then re-add the reference for D, in the event that
D
> > was specified as a reference/const-reference? (unique_ptr permits this for
> > deleters)
> 
> If I'm correct, right now we would just fail if D is a references type
correct?
> 
> In that case, we're fulfilling the "strict unique_ptr<> subset" goal that
> gromer@ was talking about. I'm okay with that since I don't think we have a
> usecase needing a reference deleter type and fixing it requires making the
> scoped_ptr(T*, d) constructor much more complicated.

Yup.

Being able to take pointer types "would be nice", but is not a must-have.

https://codereview.chromium.org/11149006/diff/40024/base/memory/scoped_ptr.h#...
base/memory/scoped_ptr.h:270: template <class T, class D =
base::DefaultDeleter<T> >
On 2012/12/12 02:17:03, awong wrote:
> On 2012/11/29 01:42:26, Ryan Sleevi wrote:
> > 20.7.1.2.1 requires that D be a function object type, lvalue-reference to
> > function, or lvalue-reference to a function object type.
> > 
> > Meaning, in particular, that D should not be a pointer type, AIUI. This is
> > enforced in libcxx, not sure about libstdc++.
> 
> Right now, I think what happens is we don't support function pointers (and I'm
> expilcitly ignoreing D& types).
> 
> We could support functions pointers by doing something like:
> 
> template <class T>
> class scoped_ptr<T, void(T*)> { ... do something special ... };
> 
> but I want to hold off on this until we have a use case.

You could use a helper type to add a "deleter_type" typedef to the class.
If is_reference<D>, then deleter_type = typename remove_reference<D>.
If is_pointer<D>, then deleter_type == typename D.
Else, deleter_type = typename add_reference<D>

then have get_deleter() and friends return deleter_type

Maybe it's more complex than that though.

https://codereview.chromium.org/11149006/diff/40024/base/memory/scoped_ptr.h#...
base/memory/scoped_ptr.h:294: scoped_ptr(scoped_ptr<U, V> other) :
impl_(&other.impl_) { }
On 2012/12/12 02:17:03, awong wrote:
> On 2012/12/04 19:41:18, gromer wrote:
> > On 2012/11/29 01:42:26, Ryan Sleevi wrote:
> > > Note 20.7.1.2.1.19, which discards the move constructor from overload
> > resolution
> > > unless:
> > > - D is a reference type and E (V) is the same type as D, or D is not a
> > reference
> > > type and E (V) is implicitly convertable to D.
> > > 
> > > 
> > > In particular, and unless I'm missing something, this seems to allow you
to
> > have
> > > a scoped_ptr<A[]> be released into a scoped_ptr<B>, assuming A and B are
> > > convertable - which would be bad, right?
> > > 
> > 
> > As noted, the simulated move semantics are outside my expertise, but FWIW
this
> > concern sounds plausible to me.
> > 
> > > If not, why specialize scoped_ptr<A[]> at all (line 380), and not just
> instead
> > > rely on the specialization of DefaultDeleter (line 129)?
> > 
> > scoped_ptr<A[]> has a different method set than the base template; in
> > particular, it lacks operator* and operator->, and provides operator[]
> instead.
> 
> Ick...nice catch.
> 
> Reading through 20.7.1.2.1.{15..17} and 20.7.1.2.1.{18..21} for
> unique_ptr(unique_ptr&& u) and template <typename U, E>
> unique_ptr(unique_ptr<U,E> && u), I think the distinction between the two
> constructors only exist if
> 
>   (a) U is an array type
>   (b) D is a reference type
> 
> If U is an array type, it shouldn't participate which will result in a compile
> failure.  In our impl, a COMPILE_ASSERT should give equivalent safety.
> 
> If we disallow D being a reference type, then by spec we technically should
> resolve to unique_ptr(unique_ptr&& u).  However, the only behavioral
difference
> is in the handling of D being a reference so I think we're okay not creating a
> second constructor.
> 
> Does that sound right to peoples?

Sounds right to me.

> 
> BTW, just tested passing from scoped_ptr<T[]> to scoped_ptr<T*>. Apparently we
> were inadvertently saved by scoped_ptr<T[]> not friending the impl class. 
Still
> putting in the COMPILE_ASSERTs just to be explicit.

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

https://codereview.chromium.org/11149006/diff/44004/base/memory/scoped_ptr.h#...
base/memory/scoped_ptr.h:170: class scoped_ptr_impl {
On 2012/12/12 02:17:03, awong wrote:
> On 2012/11/29 02:42:27, Ryan Sleevi wrote:
> > Have you confirmed what the compile time implications are of having this
> common
> > template interface?
> > 
> > I realize the desire to favour readability, but it seems like you're forcing
> > potentially four template instantiations during resolution. That is, for any
> > given T, it needs to see scoped_ptr<T,D>, which requires instantiation
> > scoped_ptr_impl<T,D>, then see if the specialization of scoped_ptr<T[],D>
> fits,
> > which requires instantiation of scoped_ptr_impl<T[],D>. At least, AIUI the
> > current resolution processes in clang and MSVC.
> > 
> > It seems that not having a common base, as admittedly harmful as that may be
> to
> > readability, and just having the two overloads (T and T[]), combined with
the
> > two deleters (D<V> and D<V[]>), may make a
> slight-in-isolation-but-large-overall
> > perf impact.
> > 
> > May be micro-optimization, but given that tricks like SFINAE-ing the Pass()
> > implementation for Callback() caused a 3% MSVC regression, or the extra arg
> for
> > base::Bind was something like a 16% regression, I want to make sure we're
not
> > going too crazy.
> 
> I'm not quite sure how to go about testing this.  Do you just run time on
winja?

Usually just 3-5 timed clobber builds, run by script, on the major platforms we
care about - once without change and once with - and see what (if any)
regression in the mean.

> 
> My gut tells me this will not nearly be as expensive. Bind() is a bit
degenerate
> because of the N^2 template specializations. However, that's gut feel without
> data.

https://codereview.chromium.org/11149006/diff/44004/base/memory/scoped_ptr.h#...
base/memory/scoped_ptr.h:217: // on the current sequencing.
On 2012/12/12 02:17:03, awong wrote:
> On 2012/12/04 19:41:18, gromer wrote:
> > Maybe add a tracking bug for this too?
> 
> I think http://crbug.com/162971 cited earlier covers this, or are you seeing
> something else?

Ah, I just fail at reading comprehension then.

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

https://codereview.chromium.org/11149006/diff/61001/base/memory/scoped_ptr.h#...
base/memory/scoped_ptr.h:300: // deleteres, function pointers as deleteres, and
deleters with reference
spelling nit: deleteres -> deleters

[gromer, 2012-12-12 17:29:57]

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

https://codereview.chromium.org/11149006/diff/40024/base/memory/scoped_ptr.h#...
base/memory/scoped_ptr.h:119: // All default single-object deleters can
trivially convert to one another.
On 2012/12/12 02:17:03, awong wrote:
> On 2012/12/04 20:39:27, gromer wrote:
> > On 2012/12/04 20:09:51, Ryan Sleevi wrote:
> > > On 2012/12/04 19:41:18, gromer wrote:
> > > > On 2012/11/29 01:42:26, Ryan Sleevi wrote:
> > > > > 20.7.1.1.2 2 has
> > > > > 
> > > > > "This constructor shall not participate in overload resolution unless
U*
> > is
> > > > > implicitly convertable to T*."
> > > > > 
> > > > > I don't see anything in this class participating in that distinction.
> > > > > 
> > > > > I realize scoped_ptr_impl is probably guaranteeing this, by virtue of
> the
> > > > > implicit conversion in scoped_ptr_impl::reset/scoped_ptr_impl::swap,
so
> > it's
> > > > > probably a non-issue, but worth noting.
> > > > > 
> > > > > It seems like we can use an enable_if trick to ensure is_convertable,
> and
> > > then
> > > > > let SFINAE remove this from the overload set if T/U are incomplete or
> > > > > non-convertable types.
> > > > 
> > > > Oh, I see, this is an issue only because of the Chromium scoped_ptr's
> > support
> > > > for move-emulation, so it doesn't apply to the Google-internal one.
> ajwong,
> > > this
> > > > one's in your court. FWIW I don't think SFINAE tricks are called for
here,
> > but
> > > a
> > > > comment might be appropriate.
> > > > 
> > > > More generally, it might make sense to explicitly document that
> > DefaultDeleter
> > > > is an implementation detail of scoped_ptr, and should not be explicitly
> used
> > > by
> > > > client code.
> > > 
> > > gromer: I'm not sure I follow your remark. The spec comment is about the
> > > constructor, but as implemented, the implicit type conversion isn't tested
> > until
> > > invoking operator()(T*), and finding that the U* originally used (for
> > > DefaultDeleter<U>) doesn't allow conversion to T* - hence, operator()(U*)
is
> > > unresolved.
> > > 
> > > I agree that it's "mostly the same", but the note about 20.7.1.1.2.1 seems
> > > applicable.
> > 
> > I'm afraid I don't follow you either; I was mostly agreeing with you that
this
> > is "probably a non-issue, but worth noting". Let me try to rephrase:
> > 
> > First of all, this isn't really my department since it's not an issue for
the
> > scoped_ptr that I maintain, so my comments should be considered advisory at
> > best.
> > 
> > That being said, my hunch is that it would be difficult to write client code
> > where the presence or absence of this SFINAE trick makes a visible
difference,
> > and all but impossible to do so without making explicit mention of
> > DefaultDeleter. I'm open to refutation; what would be especially helpful is
> > example client code that is affected by the issue.
> > 
> > That being the case, my recommendation would to add a comment to discourage
> > independent use of DefaultDeleter, but not worry about the extra mess that
> > SFINAE tricks would entail. Remember, this is DefaultDeleter, not
> > std::default_delete; we only need to worry about inconsistency with the
> standard
> > to the extent that the difference can lead to actual bugs, and/or make it
> harder
> > to port scoped_ptr clients to unique_ptr in the future
> 
> Reading through this, I think adding a COMPILE_ASSERT on on the result of
> implicit_cast which should force the right typing constraints.

LG.

https://codereview.chromium.org/11149006/diff/40024/base/memory/scoped_ptr.h#...
base/memory/scoped_ptr.h:294: scoped_ptr(scoped_ptr<U, V> other) :
impl_(&other.impl_) { }
On 2012/12/12 02:17:03, awong wrote:
> On 2012/12/04 19:41:18, gromer wrote:
> > On 2012/11/29 01:42:26, Ryan Sleevi wrote:
> > > Note 20.7.1.2.1.19, which discards the move constructor from overload
> > resolution
> > > unless:
> > > - D is a reference type and E (V) is the same type as D, or D is not a
> > reference
> > > type and E (V) is implicitly convertable to D.
> > > 
> > > 
> > > In particular, and unless I'm missing something, this seems to allow you
to
> > have
> > > a scoped_ptr<A[]> be released into a scoped_ptr<B>, assuming A and B are
> > > convertable - which would be bad, right?
> > > 
> > 
> > As noted, the simulated move semantics are outside my expertise, but FWIW
this
> > concern sounds plausible to me.
> > 
> > > If not, why specialize scoped_ptr<A[]> at all (line 380), and not just
> instead
> > > rely on the specialization of DefaultDeleter (line 129)?
> > 
> > scoped_ptr<A[]> has a different method set than the base template; in
> > particular, it lacks operator* and operator->, and provides operator[]
> instead.
> 
> Ick...nice catch.
> 
> Reading through 20.7.1.2.1.{15..17} and 20.7.1.2.1.{18..21} for
> unique_ptr(unique_ptr&& u) and template <typename U, E>
> unique_ptr(unique_ptr<U,E> && u), I think the distinction between the two
> constructors only exist if
> 
>   (a) U is an array type
>   (b) D is a reference type
> 
> If U is an array type, it shouldn't participate which will result in a compile
> failure.  In our impl, a COMPILE_ASSERT should give equivalent safety.
> 
> If we disallow D being a reference type, then by spec we technically should
> resolve to unique_ptr(unique_ptr&& u).  However, the only behavioral
difference
> is in the handling of D being a reference so I think we're okay not creating a
> second constructor.
> 
> Does that sound right to peoples?
> 
> BTW, just tested passing from scoped_ptr<T[]> to scoped_ptr<T*>. Apparently we
> were inadvertently saved by scoped_ptr<T[]> not friending the impl class. 
Still
> putting in the COMPILE_ASSERTs just to be explicit.

LG.

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

https://codereview.chromium.org/11149006/diff/61001/base/memory/scoped_ptr.h#...
base/memory/scoped_ptr.h:163: // Never allow someone to delcare something like
scoped_ptr<int[10]>.
Is there any particular reason to enforce this? I'm pretty sure it won't work
anyway (and in the unlikely event that it does, why not let it?)

https://codereview.chromium.org/11149006/diff/61001/base/memory/scoped_ptr.h#...
base/memory/scoped_ptr.h:296: // sizeof(scoped_ptr<T>) == sizeof(T*)
Might want to give a little more detail here: sizeof(scoped_ptr<T,D>) ==
sizeof(T*) iff D is an empty class. Also, be aware that strictly speaking a C++
implementation is not required to provide this property; do you have tests to
enforce it?

https://codereview.chromium.org/11149006/diff/61001/base/memory/scoped_ptr.h#...
base/memory/scoped_ptr.h:326: // Note: C++11 unique_ptr<> keeps this destructor
distinct from
destructor -> constructor

https://codereview.chromium.org/11149006/diff/61001/base/memory/scoped_ptr.h#...
base/memory/scoped_ptr.h:326: // Note: C++11 unique_ptr<> keeps this destructor
distinct from
I'm not sure this comment is necessary; readers who worry about this issue will
be few and far between, and this will just add clutter for the rest of them. If
you keep it, at least make it "Implementation note:" so clients know they can
ignore it.

https://codereview.chromium.org/11149006/diff/61001/base/memory/scoped_ptr.h#...
base/memory/scoped_ptr.h:343: // Note: C++11 unique_ptr<> keeps this operator=
distinct from
Same here.

https://codereview.chromium.org/11149006/diff/61001/base/memory/scoped_ptr.h#...
base/memory/scoped_ptr.h:344: // the normal move operator. By C++11
20.7.1.2.3.4, this templated form has
move -> move assignment

[awong, 2012-12-12 21:00:13]

Comments address.  sizeof() test added to unittest and running through trybots
now.

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

https://codereview.chromium.org/11149006/diff/40024/base/memory/scoped_ptr.h#...
base/memory/scoped_ptr.h:270: template <class T, class D =
base::DefaultDeleter<T> >
On 2012/12/12 03:37:46, Ryan Sleevi wrote:
> On 2012/12/12 02:17:03, awong wrote:
> > On 2012/11/29 01:42:26, Ryan Sleevi wrote:
> > > 20.7.1.2.1 requires that D be a function object type, lvalue-reference to
> > > function, or lvalue-reference to a function object type.
> > > 
> > > Meaning, in particular, that D should not be a pointer type, AIUI. This is
> > > enforced in libcxx, not sure about libstdc++.
> > 
> > Right now, I think what happens is we don't support function pointers (and
I'm
> > expilcitly ignoreing D& types).
> > 
> > We could support functions pointers by doing something like:
> > 
> > template <class T>
> > class scoped_ptr<T, void(T*)> { ... do something special ... };
> > 
> > but I want to hold off on this until we have a use case.
> 
> You could use a helper type to add a "deleter_type" typedef to the class.
> If is_reference<D>, then deleter_type = typename remove_reference<D>.
> If is_pointer<D>, then deleter_type == typename D.
> Else, deleter_type = typename add_reference<D>
> 
> then have get_deleter() and friends return deleter_type
> 
> Maybe it's more complex than that though.

Yeah, I think that would work. However, if I start handling reference types for
D, then the move constructor and move assignment get far far more complex.  I'm
happier having things just explode for now with a reference type :P

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

https://codereview.chromium.org/11149006/diff/44004/base/memory/scoped_ptr.h#...
base/memory/scoped_ptr.h:170: class scoped_ptr_impl {
On 2012/12/12 03:37:47, Ryan Sleevi wrote:
> On 2012/12/12 02:17:03, awong wrote:
> > On 2012/11/29 02:42:27, Ryan Sleevi wrote:
> > > Have you confirmed what the compile time implications are of having this
> > common
> > > template interface?
> > > 
> > > I realize the desire to favour readability, but it seems like you're
forcing
> > > potentially four template instantiations during resolution. That is, for
any
> > > given T, it needs to see scoped_ptr<T,D>, which requires instantiation
> > > scoped_ptr_impl<T,D>, then see if the specialization of scoped_ptr<T[],D>
> > fits,
> > > which requires instantiation of scoped_ptr_impl<T[],D>. At least, AIUI the
> > > current resolution processes in clang and MSVC.
> > > 
> > > It seems that not having a common base, as admittedly harmful as that may
be
> > to
> > > readability, and just having the two overloads (T and T[]), combined with
> the
> > > two deleters (D<V> and D<V[]>), may make a
> > slight-in-isolation-but-large-overall
> > > perf impact.
> > > 
> > > May be micro-optimization, but given that tricks like SFINAE-ing the
Pass()
> > > implementation for Callback() caused a 3% MSVC regression, or the extra
arg
> > for
> > > base::Bind was something like a 16% regression, I want to make sure we're
> not
> > > going too crazy.
> > 
> > I'm not quite sure how to go about testing this.  Do you just run time on
> winja?
> 
> Usually just 3-5 timed clobber builds, run by script, on the major platforms
we
> care about - once without change and once with - and see what (if any)
> regression in the mean.
> 
> > 
> > My gut tells me this will not nearly be as expensive. Bind() is a bit
> degenerate
> > because of the N^2 template specializations. However, that's gut feel
without
> > data.
> 

Grrr...can I just test on the build bots? :D

(I don't actually have working builds on any platform than Linux)

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

https://codereview.chromium.org/11149006/diff/61001/base/memory/scoped_ptr.h#...
base/memory/scoped_ptr.h:163: // Never allow someone to delcare something like
scoped_ptr<int[10]>.
On 2012/12/12 17:29:57, gromer wrote:
> Is there any particular reason to enforce this? I'm pretty sure it won't work
> anyway (and in the unlikely event that it does, why not let it?)

I think without this, you actually do the wrong thing.

scoped_ptr<int[10]> goes to DefaultDeleter<int[10]> which would resolve by
default to the single-object version above because it doesn't match the
specialization.

https://codereview.chromium.org/11149006/diff/61001/base/memory/scoped_ptr.h#...
base/memory/scoped_ptr.h:296: // sizeof(scoped_ptr<T>) == sizeof(T*)
On 2012/12/12 17:29:57, gromer wrote:
> Might want to give a little more detail here: sizeof(scoped_ptr<T,D>) ==
> sizeof(T*) iff D is an empty class. Also, be aware that strictly speaking a
C++
> implementation is not required to provide this property; do you have tests to
> enforce it?

Referred people to the scoped_ptr_impl<> comment.  Also, test added...let's see
what breaks.

https://codereview.chromium.org/11149006/diff/61001/base/memory/scoped_ptr.h#...
base/memory/scoped_ptr.h:300: // deleteres, function pointers as deleteres, and
deleters with reference
On 2012/12/12 03:37:47, Ryan Sleevi wrote:
> spelling nit: deleteres -> deleters

Done.

https://codereview.chromium.org/11149006/diff/61001/base/memory/scoped_ptr.h#...
base/memory/scoped_ptr.h:326: // Note: C++11 unique_ptr<> keeps this destructor
distinct from
On 2012/12/12 17:29:57, gromer wrote:
> destructor -> constructor

Done.

https://codereview.chromium.org/11149006/diff/61001/base/memory/scoped_ptr.h#...
base/memory/scoped_ptr.h:326: // Note: C++11 unique_ptr<> keeps this destructor
distinct from
On 2012/12/12 17:29:57, gromer wrote:
> I'm not sure this comment is necessary; readers who worry about this issue
will
> be few and far between, and this will just add clutter for the rest of them.
If
> you keep it, at least make it "Implementation note:" so clients know they can
> ignore it.

I changed it to IMPLEMENTATION NOTE.  I want to keep the comment because it's
subtle enough that it would almost certainly be missed by someone casually
modifying the code.

https://codereview.chromium.org/11149006/diff/61001/base/memory/scoped_ptr.h#...
base/memory/scoped_ptr.h:343: // Note: C++11 unique_ptr<> keeps this operator=
distinct from
On 2012/12/12 17:29:57, gromer wrote:
> Same here.

Done.

https://codereview.chromium.org/11149006/diff/61001/base/memory/scoped_ptr.h#...
base/memory/scoped_ptr.h:344: // the normal move operator. By C++11
20.7.1.2.3.4, this templated form has
On 2012/12/12 17:29:57, gromer wrote:
> move -> move assignment

Done.

[gromer, 2012-12-14 16:57:26]

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

https://codereview.chromium.org/11149006/diff/61001/base/memory/scoped_ptr.h#...
base/memory/scoped_ptr.h:163: // Never allow someone to delcare something like
scoped_ptr<int[10]>.
On 2012/12/12 21:00:14, awong wrote:
> On 2012/12/12 17:29:57, gromer wrote:
> > Is there any particular reason to enforce this? I'm pretty sure it won't
work
> > anyway (and in the unlikely event that it does, why not let it?)
> 
> I think without this, you actually do the wrong thing.
> 
> scoped_ptr<int[10]> goes to DefaultDeleter<int[10]> which would resolve by
> default to the single-object version above because it doesn't match the
> specialization.

Still, it's odd that std::unique_ptr doesn't make this provision. Are you able
to demonstrate this problem in actual code?

https://codereview.chromium.org/11149006/diff/61001/base/memory/scoped_ptr.h#...
base/memory/scoped_ptr.h:296: // sizeof(scoped_ptr<T>) == sizeof(T*)
On 2012/12/12 21:00:14, awong wrote:
> On 2012/12/12 17:29:57, gromer wrote:
> > Might want to give a little more detail here: sizeof(scoped_ptr<T,D>) ==
> > sizeof(T*) iff D is an empty class. Also, be aware that strictly speaking a
> C++
> > implementation is not required to provide this property; do you have tests
to
> > enforce it?
> 
> Referred people to the scoped_ptr_impl<> comment.  Also, test added...let's
see
> what breaks.

"size ... is small" is redundant, and it'd be good to at least slightly hedge
the claim that they're equal. How about "A scoped ptr is small; usually,
sizeof..."

https://codereview.chromium.org/11149006/diff/61001/base/memory/scoped_ptr.h#...
base/memory/scoped_ptr.h:344: // the normal move operator. By C++11
20.7.1.2.3.4, this templated form has
On 2012/12/12 21:00:14, awong wrote:
> On 2012/12/12 17:29:57, gromer wrote:
> > move -> move assignment
> 
> Done.

Still need to fix line 345

https://codereview.chromium.org/11149006/diff/66001/base/memory/scoped_ptr_un...
File base/memory/scoped_ptr_unittest.cc (right):

https://codereview.chromium.org/11149006/diff/66001/base/memory/scoped_ptr_un...
base/memory/scoped_ptr_unittest.cc:106: scoped_ptr_larger_than_raw_ptr);
Absurd nitpick: "larger than" doesn't match the actual assertion; maybe the
comparison should be >=?

[awong, 2012-12-14 20:25:26]

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

https://codereview.chromium.org/11149006/diff/61001/base/memory/scoped_ptr.h#...
base/memory/scoped_ptr.h:163: // Never allow someone to delcare something like
scoped_ptr<int[10]>.
On 2012/12/14 16:57:26, gromer wrote:
> On 2012/12/12 21:00:14, awong wrote:
> > On 2012/12/12 17:29:57, gromer wrote:
> > > Is there any particular reason to enforce this? I'm pretty sure it won't
> work
> > > anyway (and in the unlikely event that it does, why not let it?)
> > 
> > I think without this, you actually do the wrong thing.
> > 
> > scoped_ptr<int[10]> goes to DefaultDeleter<int[10]> which would resolve by
> > default to the single-object version above because it doesn't match the
> > specialization.
> 
> Still, it's odd that std::unique_ptr doesn't make this provision. Are you able
> to demonstrate this problem in actual code?

Kinda...

If I remove this code and then do:

int array[10];
scoped_ptr<int[10]> x(&array);

Clang dies with the following error:
In file included from ../../base/memory/scoped_ptr_unittest.cc:5:
../../base/memory/scoped_ptr.h:138:5: error: 'delete' applied to a
pointer-to-array type 'int *[10]' treated as delete[] [-Werror]
    delete ptr;
    ^      ~~~
          []
../../base/memory/scoped_ptr.h:215:7: note: in instantiation of member function
'base::DefaultDeleter<int [10]>::operator()' requested here
      static_cast<D&>(data_)(data_.ptr);


Also, I have been unable to actually construct a "new expression" that would
validly create a pointer-to-sized-array.  I'm starting to believe it is
impossible...but I haven't found langauge stating it definitely.

On the otherhand, this explcitly is possible

   scoped_ptr<int[10]> x(new [m][10]);

By 5.3.4.5 [expr.new]p5, the type of "new [m][10]" is int(*)[10] which would
match and then do the wrong thing if the delete call actually didn't give a
warning.

Assuming there is no way to use a single-object new expression to create an
int*[10], this feels like it might a hole in the default_deleter spec...

https://codereview.chromium.org/11149006/diff/61001/base/memory/scoped_ptr.h#...
base/memory/scoped_ptr.h:296: // sizeof(scoped_ptr<T>) == sizeof(T*)
On 2012/12/14 16:57:26, gromer wrote:
> On 2012/12/12 21:00:14, awong wrote:
> > On 2012/12/12 17:29:57, gromer wrote:
> > > Might want to give a little more detail here: sizeof(scoped_ptr<T,D>) ==
> > > sizeof(T*) iff D is an empty class. Also, be aware that strictly speaking
a
> > C++
> > > implementation is not required to provide this property; do you have tests
> to
> > > enforce it?
> > 
> > Referred people to the scoped_ptr_impl<> comment.  Also, test added...let's
> see
> > what breaks.
> 
> "size ... is small" is redundant, and it'd be good to at least slightly hedge
> the claim that they're equal. How about "A scoped ptr is small; usually,
> sizeof..."

Hedged it by saying "on most compilers"

https://codereview.chromium.org/11149006/diff/61001/base/memory/scoped_ptr.h#...
base/memory/scoped_ptr.h:344: // the normal move operator. By C++11
20.7.1.2.3.4, this templated form has
On 2012/12/14 16:57:26, gromer wrote:
> On 2012/12/12 21:00:14, awong wrote:
> > On 2012/12/12 17:29:57, gromer wrote:
> > > move -> move assignment
> > 
> > Done.
> 
> Still need to fix line 345

Done.

[willchan no longer on Chromium, 2012-12-14 21:07:03]

There are enough C++ ninjas (all who know it far better than I) reviewing this
CL that I'm not going to bother to review it as long as people assert:
* This is strictly moving us towards closer compatibility with unique_ptr
* The changelist description is up to date with all the discussion/changes
happening in this thread.

Ping me whenever my OWNERS rubberstamp is needed.

[gromer, 2012-12-14 21:11:46]

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

https://codereview.chromium.org/11149006/diff/61001/base/memory/scoped_ptr.h#...
base/memory/scoped_ptr.h:163: // Never allow someone to delcare something like
scoped_ptr<int[10]>.
On 2012/12/14 20:25:26, awong wrote:
> On 2012/12/14 16:57:26, gromer wrote:
> > On 2012/12/12 21:00:14, awong wrote:
> > > On 2012/12/12 17:29:57, gromer wrote:
> > > > Is there any particular reason to enforce this? I'm pretty sure it won't
> > work
> > > > anyway (and in the unlikely event that it does, why not let it?)
> > > 
> > > I think without this, you actually do the wrong thing.
> > > 
> > > scoped_ptr<int[10]> goes to DefaultDeleter<int[10]> which would resolve by
> > > default to the single-object version above because it doesn't match the
> > > specialization.
> > 
> > Still, it's odd that std::unique_ptr doesn't make this provision. Are you
able
> > to demonstrate this problem in actual code?
> 
> Kinda...
> 
> If I remove this code and then do:
> 
> int array[10];
> scoped_ptr<int[10]> x(&array);
> 
> Clang dies with the following error:
> In file included from ../../base/memory/scoped_ptr_unittest.cc:5:
> ../../base/memory/scoped_ptr.h:138:5: error: 'delete' applied to a
> pointer-to-array type 'int *[10]' treated as delete[] [-Werror]
>     delete ptr;
>     ^      ~~~
>           []
> ../../base/memory/scoped_ptr.h:215:7: note: in instantiation of member
function
> 'base::DefaultDeleter<int [10]>::operator()' requested here
>       static_cast<D&>(data_)(data_.ptr);
> 
> 
> Also, I have been unable to actually construct a "new expression" that would
> validly create a pointer-to-sized-array.  I'm starting to believe it is
> impossible...but I haven't found langauge stating it definitely.
> 
> On the otherhand, this explcitly is possible
> 
>    scoped_ptr<int[10]> x(new [m][10]);
> 
> By 5.3.4.5 [expr.new]p5, the type of "new [m][10]" is int(*)[10] which would
> match and then do the wrong thing if the delete call actually didn't give a
> warning.

I'm confused; doesn't this answer your question? Isn't "new [m][10]" a new
expression that validly creates a pointer-to-sized-array?

> 
> Assuming there is no way to use a single-object new expression to create an
> int*[10], this feels like it might a hole in the default_deleter spec...

Yeah, considering all you hit was a warning, not a mandatory diagnostic, this
looks like a hole in the standard. I'll ask on the LWG reflector, and I withdraw
my objection to this code.

https://codereview.chromium.org/11149006/diff/61001/base/memory/scoped_ptr.h#...
base/memory/scoped_ptr.h:296: // sizeof(scoped_ptr<T>) == sizeof(T*)
On 2012/12/14 20:25:26, awong wrote:
> On 2012/12/14 16:57:26, gromer wrote:
> > On 2012/12/12 21:00:14, awong wrote:
> > > On 2012/12/12 17:29:57, gromer wrote:
> > > > Might want to give a little more detail here: sizeof(scoped_ptr<T,D>) ==
> > > > sizeof(T*) iff D is an empty class. Also, be aware that strictly
speaking
> a
> > > C++
> > > > implementation is not required to provide this property; do you have
tests
> > to
> > > > enforce it?
> > > 
> > > Referred people to the scoped_ptr_impl<> comment.  Also, test
added...let's
> > see
> > > what breaks.
> > 
> > "size ... is small" is redundant, and it'd be good to at least slightly
hedge
> > the claim that they're equal. How about "A scoped ptr is small; usually,
> > sizeof..."
> 
> Hedged it by saying "on most compilers"

I feel like this still risks giving the impression that this remains true for
arbitrary deleters, which is definitely not the case.

[awong, 2012-12-14 21:18:39]

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

https://codereview.chromium.org/11149006/diff/61001/base/memory/scoped_ptr.h#...
base/memory/scoped_ptr.h:163: // Never allow someone to delcare something like
scoped_ptr<int[10]>.
On 2012/12/14 21:11:46, gromer wrote:
> On 2012/12/14 20:25:26, awong wrote:
> > On 2012/12/14 16:57:26, gromer wrote:
> > > On 2012/12/12 21:00:14, awong wrote:
> > > > On 2012/12/12 17:29:57, gromer wrote:
> > > > > Is there any particular reason to enforce this? I'm pretty sure it
won't
> > > work
> > > > > anyway (and in the unlikely event that it does, why not let it?)
> > > > 
> > > > I think without this, you actually do the wrong thing.
> > > > 
> > > > scoped_ptr<int[10]> goes to DefaultDeleter<int[10]> which would resolve
by
> > > > default to the single-object version above because it doesn't match the
> > > > specialization.
> > > 
> > > Still, it's odd that std::unique_ptr doesn't make this provision. Are you
> able
> > > to demonstrate this problem in actual code?
> > 
> > Kinda...
> > 
> > If I remove this code and then do:
> > 
> > int array[10];
> > scoped_ptr<int[10]> x(&array);
> > 
> > Clang dies with the following error:
> > In file included from ../../base/memory/scoped_ptr_unittest.cc:5:
> > ../../base/memory/scoped_ptr.h:138:5: error: 'delete' applied to a
> > pointer-to-array type 'int *[10]' treated as delete[] [-Werror]
> >     delete ptr;
> >     ^      ~~~
> >           []
> > ../../base/memory/scoped_ptr.h:215:7: note: in instantiation of member
> function
> > 'base::DefaultDeleter<int [10]>::operator()' requested here
> >       static_cast<D&>(data_)(data_.ptr);
> > 
> > 
> > Also, I have been unable to actually construct a "new expression" that would
> > validly create a pointer-to-sized-array.  I'm starting to believe it is
> > impossible...but I haven't found langauge stating it definitely.
> > 
> > On the otherhand, this explcitly is possible
> > 
> >    scoped_ptr<int[10]> x(new [m][10]);
> > 
> > By 5.3.4.5 [expr.new]p5, the type of "new [m][10]" is int(*)[10] which would
> > match and then do the wrong thing if the delete call actually didn't give a
> > warning.
> 
> I'm confused; doesn't this answer your question? Isn't "new [m][10]" a new
> expression that validly creates a pointer-to-sized-array?

Yes...but it must be matched with an delete[].  Without this compile assert, we
entered undefined behavior land per [expr.delete]p2.


> > 
> > Assuming there is no way to use a single-object new expression to create an
> > int*[10], this feels like it might a hole in the default_deleter spec...
> 
> Yeah, considering all you hit was a warning, not a mandatory diagnostic, this
> looks like a hole in the standard. I'll ask on the LWG reflector, and I
withdraw
> my objection to this code.

I am now fairly convinced that is impossible to construct a scoped_ptr<int[10]>
(or unique_ptr<int[10]>) that will "do the right thing."

Also found this:
  http://www.open-std.org/jtc1/sc22/wg21/docs/lwg-defects.html#740

which interestingly states that pointer decay would cause this to not compile
anyways, so you don't need a static_assert.  I think I disagree.

[awong, 2012-12-14 23:45:11]

Last nit addressed.  Thanks for all the detailed review and comments so far. I
think we've covered all the holes now.

I'm currently doing compile benchmarks, will post results after they're done. 
Assuming they all seem sane (I'm seeing a small degrade in build perf on win but
I think it might be noise...waiting for a few more runs to complete) then I'd
like to finish up this review and get ready for commit next week.

If you guys don't see any more issues, wanna give me some LGs? :D :D

-Albert


On 2012/12/14 21:18:39, awong wrote:
> https://codereview.chromium.org/11149006/diff/61001/base/memory/scoped_ptr.h
> File base/memory/scoped_ptr.h (right):
> 
>
https://codereview.chromium.org/11149006/diff/61001/base/memory/scoped_ptr.h#...
> base/memory/scoped_ptr.h:163: // Never allow someone to delcare something like
> scoped_ptr<int[10]>.
> On 2012/12/14 21:11:46, gromer wrote:
> > On 2012/12/14 20:25:26, awong wrote:
> > > On 2012/12/14 16:57:26, gromer wrote:
> > > > On 2012/12/12 21:00:14, awong wrote:
> > > > > On 2012/12/12 17:29:57, gromer wrote:
> > > > > > Is there any particular reason to enforce this? I'm pretty sure it
> won't
> > > > work
> > > > > > anyway (and in the unlikely event that it does, why not let it?)
> > > > > 
> > > > > I think without this, you actually do the wrong thing.
> > > > > 
> > > > > scoped_ptr<int[10]> goes to DefaultDeleter<int[10]> which would
resolve
> by
> > > > > default to the single-object version above because it doesn't match
the
> > > > > specialization.
> > > > 
> > > > Still, it's odd that std::unique_ptr doesn't make this provision. Are
you
> > able
> > > > to demonstrate this problem in actual code?
> > > 
> > > Kinda...
> > > 
> > > If I remove this code and then do:
> > > 
> > > int array[10];
> > > scoped_ptr<int[10]> x(&array);
> > > 
> > > Clang dies with the following error:
> > > In file included from ../../base/memory/scoped_ptr_unittest.cc:5:
> > > ../../base/memory/scoped_ptr.h:138:5: error: 'delete' applied to a
> > > pointer-to-array type 'int *[10]' treated as delete[] [-Werror]
> > >     delete ptr;
> > >     ^      ~~~
> > >           []
> > > ../../base/memory/scoped_ptr.h:215:7: note: in instantiation of member
> > function
> > > 'base::DefaultDeleter<int [10]>::operator()' requested here
> > >       static_cast<D&>(data_)(data_.ptr);
> > > 
> > > 
> > > Also, I have been unable to actually construct a "new expression" that
would
> > > validly create a pointer-to-sized-array.  I'm starting to believe it is
> > > impossible...but I haven't found langauge stating it definitely.
> > > 
> > > On the otherhand, this explcitly is possible
> > > 
> > >    scoped_ptr<int[10]> x(new [m][10]);
> > > 
> > > By 5.3.4.5 [expr.new]p5, the type of "new [m][10]" is int(*)[10] which
would
> > > match and then do the wrong thing if the delete call actually didn't give
a
> > > warning.
> > 
> > I'm confused; doesn't this answer your question? Isn't "new [m][10]" a new
> > expression that validly creates a pointer-to-sized-array?
> 
> Yes...but it must be matched with an delete[].  Without this compile assert,
we
> entered undefined behavior land per [expr.delete]p2.
> 
> 
> > > 
> > > Assuming there is no way to use a single-object new expression to create
an
> > > int*[10], this feels like it might a hole in the default_deleter spec...
> > 
> > Yeah, considering all you hit was a warning, not a mandatory diagnostic,
this
> > looks like a hole in the standard. I'll ask on the LWG reflector, and I
> withdraw
> > my objection to this code.
> 
> I am now fairly convinced that is impossible to construct a
scoped_ptr<int[10]>
> (or unique_ptr<int[10]>) that will "do the right thing."
> 
> Also found this:
>   http://www.open-std.org/jtc1/sc22/wg21/docs/lwg-defects.html#740
> 
> which interestingly states that pointer decay would cause this to not compile
> anyways, so you don't need a static_assert.  I think I disagree.

[awong, 2012-12-15 00:19:08]

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

https://codereview.chromium.org/11149006/diff/61001/base/memory/scoped_ptr.h#...
base/memory/scoped_ptr.h:296: // sizeof(scoped_ptr<T>) == sizeof(T*)
On 2012/12/14 21:11:46, gromer wrote:
> On 2012/12/14 20:25:26, awong wrote:
> > On 2012/12/14 16:57:26, gromer wrote:
> > > On 2012/12/12 21:00:14, awong wrote:
> > > > On 2012/12/12 17:29:57, gromer wrote:
> > > > > Might want to give a little more detail here: sizeof(scoped_ptr<T,D>)
==
> > > > > sizeof(T*) iff D is an empty class. Also, be aware that strictly
> speaking
> > a
> > > > C++
> > > > > implementation is not required to provide this property; do you have
> tests
> > > to
> > > > > enforce it?
> > > > 
> > > > Referred people to the scoped_ptr_impl<> comment.  Also, test
> added...let's
> > > see
> > > > what breaks.
> > > 
> > > "size ... is small" is redundant, and it'd be good to at least slightly
> hedge
> > > the claim that they're equal. How about "A scoped ptr is small; usually,
> > > sizeof..."
> > 
> > Hedged it by saying "on most compilers"
> 
> I feel like this still risks giving the impression that this remains true for
> arbitrary deleters, which is definitely not the case.

Added more details.

https://codereview.chromium.org/11149006/diff/66001/base/memory/scoped_ptr_un...
File base/memory/scoped_ptr_unittest.cc (right):

https://codereview.chromium.org/11149006/diff/66001/base/memory/scoped_ptr_un...
base/memory/scoped_ptr_unittest.cc:106: scoped_ptr_larger_than_raw_ptr);
On 2012/12/14 16:57:26, gromer wrote:
> Absurd nitpick: "larger than" doesn't match the actual assertion; maybe the
> comparison should be >=?

Done

[gromer, 2012-12-15 00:21:40]

lgtm

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

https://codereview.chromium.org/11149006/diff/61001/base/memory/scoped_ptr.h#...
base/memory/scoped_ptr.h:296: // sizeof(scoped_ptr<T>) == sizeof(T*)
On 2012/12/15 00:19:08, awong wrote:
> On 2012/12/14 21:11:46, gromer wrote:
> > On 2012/12/14 20:25:26, awong wrote:
> > > On 2012/12/14 16:57:26, gromer wrote:
> > > > On 2012/12/12 21:00:14, awong wrote:
> > > > > On 2012/12/12 17:29:57, gromer wrote:
> > > > > > Might want to give a little more detail here:
sizeof(scoped_ptr<T,D>)
> ==
> > > > > > sizeof(T*) iff D is an empty class. Also, be aware that strictly
> > speaking
> > > a
> > > > > C++
> > > > > > implementation is not required to provide this property; do you have
> > tests
> > > > to
> > > > > > enforce it?
> > > > > 
> > > > > Referred people to the scoped_ptr_impl<> comment.  Also, test
> > added...let's
> > > > see
> > > > > what breaks.
> > > > 
> > > > "size ... is small" is redundant, and it'd be good to at least slightly
> > hedge
> > > > the claim that they're equal. How about "A scoped ptr is small; usually,
> > > > sizeof..."
> > > 
> > > Hedged it by saying "on most compilers"
> > 
> > I feel like this still risks giving the impression that this remains true
for
> > arbitrary deleters, which is definitely not the case.
> 
> Added more details.

FWIW I would have accepted _fewer_ details, a la my "usually" suggestion, but
this is fine too.

[Ryan Sleevi, 2012-12-15 02:24:12]

LGTM!

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

https://codereview.chromium.org/11149006/diff/76002/base/memory/scoped_ptr.h#...
base/memory/scoped_ptr.h:120: // U* is implicitly convertible to T* and U is not
an array type.
nit: typo (provies -> provides)
nit: typo (of -> if)
nit: default_deleter -> default_delete
nit: instead of saying "spec for std::default_deleter"

"IMPLEMENTATION NOTE: C++11 20.7.1.1.2p2 only provides this constructor if ..."

https://codereview.chromium.org/11149006/diff/76002/base/memory/scoped_ptr.h#...
base/memory/scoped_ptr.h:123: // constructor. However, since there are no other
1-argument constructors
grammar nit: "constructors" -> "constructors,"

https://codereview.chromium.org/11149006/diff/76002/base/memory/scoped_ptr.h#...
base/memory/scoped_ptr.h:129: // array. T is guaranteed to be a non-array so any
U* where U is an array
grammar nit: "non-array so" -> "non-array, so"

https://codereview.chromium.org/11149006/diff/76002/base/memory/scoped_ptr.h#...
base/memory/scoped_ptr.h:163: // Never allow someone to delcare something like
scoped_ptr<int[10]>.
typo: delcare -> declare

https://codereview.chromium.org/11149006/diff/76002/base/memory/scoped_ptr.h#...
base/memory/scoped_ptr.h:534: void reset(U* array);
nit: unnecessary line wrapping (and on 529), particularly when compared to
540/541 and 429/430.

[awong, 2012-12-15 02:32:26]

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

https://codereview.chromium.org/11149006/diff/76002/base/memory/scoped_ptr.h#...
base/memory/scoped_ptr.h:120: // U* is implicitly convertible to T* and U is not
an array type.
On 2012/12/15 02:24:12, Ryan Sleevi wrote:
> nit: typo (provies -> provides)
> nit: typo (of -> if)
> nit: default_deleter -> default_delete
> nit: instead of saying "spec for std::default_deleter"
> 
> "IMPLEMENTATION NOTE: C++11 20.7.1.1.2p2 only provides this constructor if
..."

Done.

https://codereview.chromium.org/11149006/diff/76002/base/memory/scoped_ptr.h#...
base/memory/scoped_ptr.h:123: // constructor. However, since there are no other
1-argument constructors
On 2012/12/15 02:24:12, Ryan Sleevi wrote:
> grammar nit: "constructors" -> "constructors,"

Done.

https://codereview.chromium.org/11149006/diff/76002/base/memory/scoped_ptr.h#...
base/memory/scoped_ptr.h:129: // array. T is guaranteed to be a non-array so any
U* where U is an array
On 2012/12/15 02:24:12, Ryan Sleevi wrote:
> grammar nit: "non-array so" -> "non-array, so"

Done.

https://codereview.chromium.org/11149006/diff/76002/base/memory/scoped_ptr.h#...
base/memory/scoped_ptr.h:163: // Never allow someone to delcare something like
scoped_ptr<int[10]>.
On 2012/12/15 02:24:12, Ryan Sleevi wrote:
> typo: delcare -> declare

Done.

https://codereview.chromium.org/11149006/diff/76002/base/memory/scoped_ptr.h#...
base/memory/scoped_ptr.h:534: void reset(U* array);
On 2012/12/15 02:24:12, Ryan Sleevi wrote:
> nit: unnecessary line wrapping (and on 529), particularly when compared to
> 540/541 and 429/430.

Done.

[awong, 2012-12-18 19:59:37]

willchan: Wanna do the OWNERS honors?

I'll submit after M25 branch cut.

On 2012/12/15 02:32:26, awong wrote:
> https://codereview.chromium.org/11149006/diff/76002/base/memory/scoped_ptr.h
> File base/memory/scoped_ptr.h (right):
> 
>
https://codereview.chromium.org/11149006/diff/76002/base/memory/scoped_ptr.h#...
> base/memory/scoped_ptr.h:120: // U* is implicitly convertible to T* and U is
not
> an array type.
> On 2012/12/15 02:24:12, Ryan Sleevi wrote:
> > nit: typo (provies -> provides)
> > nit: typo (of -> if)
> > nit: default_deleter -> default_delete
> > nit: instead of saying "spec for std::default_deleter"
> > 
> > "IMPLEMENTATION NOTE: C++11 20.7.1.1.2p2 only provides this constructor if
> ..."
> 
> Done.
> 
>
https://codereview.chromium.org/11149006/diff/76002/base/memory/scoped_ptr.h#...
> base/memory/scoped_ptr.h:123: // constructor. However, since there are no
other
> 1-argument constructors
> On 2012/12/15 02:24:12, Ryan Sleevi wrote:
> > grammar nit: "constructors" -> "constructors,"
> 
> Done.
> 
>
https://codereview.chromium.org/11149006/diff/76002/base/memory/scoped_ptr.h#...
> base/memory/scoped_ptr.h:129: // array. T is guaranteed to be a non-array so
any
> U* where U is an array
> On 2012/12/15 02:24:12, Ryan Sleevi wrote:
> > grammar nit: "non-array so" -> "non-array, so"
> 
> Done.
> 
>
https://codereview.chromium.org/11149006/diff/76002/base/memory/scoped_ptr.h#...
> base/memory/scoped_ptr.h:163: // Never allow someone to delcare something like
> scoped_ptr<int[10]>.
> On 2012/12/15 02:24:12, Ryan Sleevi wrote:
> > typo: delcare -> declare
> 
> Done.
> 
>
https://codereview.chromium.org/11149006/diff/76002/base/memory/scoped_ptr.h#...
> base/memory/scoped_ptr.h:534: void reset(U* array);
> On 2012/12/15 02:24:12, Ryan Sleevi wrote:
> > nit: unnecessary line wrapping (and on 529), particularly when compared to
> > 540/541 and 429/430.
> 
> Done.

[willchan no longer on Chromium, 2012-12-18 23:01:35]

Please make sure that the changelist description is up to date with everything
y'all discussed.

Otherwise, LGTM.

[awong, 2012-12-19 00:03:36]

Updated description with a list of known deficiencies.

[awong, 2012-12-19 20:56:57]

On 2012/12/19 00:03:36, awong wrote:
> Updated description with a list of known deficiencies.

TBR dhollowa,kinuko,ananta,hans,scherkus since it's changing your friends.

[commit-bot: I haz the power, 2012-12-19 20:58:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ajwong@chromium.org/11149006/85001

[commit-bot: I haz the power, 2012-12-19 21:01:11]

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-12-19 21:08:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ajwong@chromium.org/11149006/85001