Add WrapUnique helper to wrap a raw pointer in a std::unique_ptr.

Add WrapUnique helper to wrap a raw pointer in a std::unique_ptr.

This is the equivalent of make_scoped_ptr in a post-scoped_ptr world. It
is not called MakeUniquePtr because C++14 adds a std::make_unique helper
with different semantics.

BUG=554522
Committed: https://crrev.com/2ba07d1f31f04391c83a53a2e49387de58b1ac2d
Cr-Commit-Position: refs/heads/master@{#365079}

[dcheng, 2015-12-11 09:14:14]

dcheng@chromium.org changed reviewers:
+ danakj@chromium.org, jyasskin@chromium.org, thakis@chromium.org

[dcheng, 2015-12-11 21:32:01]

Another thing to consider: is it worth adding no-compile tests for the static
asserts?

nocompile tests are essentially a second-class construct in the current build
system.

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

https://codereview.chromium.org/1513423002/diff/1/base/memory/ptr_util.h#newc...
base/memory/ptr_util.h:16: static_assert(!std::is_array<T>::value ||
std::extent<T>::value != 0,
After puzzling over this, I still don't understand the reasoning behind these
static asserts. make_scoped_ptr (both in Chromium and in google3) never had this
assert, but the google3 equivalent of this helper has these.

Since this template is specialized on T*, this can only match pointers to
arrays.

So T(*)[], T(*)[n], and T(*)[0] (as far as I understand).



My rough understanding is that these static asserts are to keep this helper from
returning a unique_ptr<T[]> rather than a unique_ptr<T>.

Two main things I don't understand:
1. how you would get an expression of this type that you'd want to store in a
unique_ptr<T>.

So for example:
char s[] = "abc";
WrapUnique(&s);
compiles, even though it's clearly wrong.

Maybe a multi-dimensional array?

2. why is it OK to delete a T(*)[n] through scalar delete? I'm sure I'm missing
something obvious here...

[danakj, 2015-12-11 22:01:33]

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

https://codereview.chromium.org/1513423002/diff/1/base/memory/ptr_util.h#newc...
base/memory/ptr_util.h:15: std::unique_ptr<T> WrapUnique(T* ptr) {
I don't understand the point of WrapUnique(T*), there's already
std::unique_ptr<T>(T*);

I was thinking we would like to have more like

base::MakeUnique(a, b, c) -> std::unique_ptr(new T(a, b, c));

?

[dcheng, 2015-12-11 22:54:26]

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

https://codereview.chromium.org/1513423002/diff/1/base/memory/ptr_util.h#newc...
base/memory/ptr_util.h:15: std::unique_ptr<T> WrapUnique(T* ptr) {
On 2015/12/11 at 22:01:32, danakj (behind on reviews) wrote:
> I don't understand the point of WrapUnique(T*), there's already
std::unique_ptr<T>(T*);
> 
> I was thinking we would like to have more like
> 
> base::MakeUnique(a, b, c) -> std::unique_ptr(new T(a, b, c));
> 
> ?

I think in a world where every owned pointer was returned as a unique_ptr, we
wouldn't need this.

But that's not the case, and I think that's where this helper comes in handy.
People should definitely prefer MakeUnique where possible (e.g. WrapUnique(new
T(a, b, c)) should be written MakeUnique<T>(a, b, c)) but there are still a lot
of ownership-transferring raw pointers.

I wanted to split this up from actually adding MakeUnique, since there's
probably going to be some more interesting discussion on MakeUnique itself.

[danakj, 2015-12-12 00:01:23]

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

https://codereview.chromium.org/1513423002/diff/1/base/memory/ptr_util.h#newc...
base/memory/ptr_util.h:16: static_assert(!std::is_array<T>::value ||
std::extent<T>::value != 0,
On 2015/12/11 21:32:01, dcheng wrote:
> After puzzling over this, I still don't understand the reasoning behind these
> static asserts. make_scoped_ptr (both in Chromium and in google3) never had
this
> assert, but the google3 equivalent of this helper has these.
> 
> Since this template is specialized on T*, this can only match pointers to
> arrays.
> 
> So T(*)[], T(*)[n], and T(*)[0] (as far as I understand).
> 
> 
> 
> My rough understanding is that these static asserts are to keep this helper
from
> returning a unique_ptr<T[]> rather than a unique_ptr<T>.
> 
> Two main things I don't understand:
> 1. how you would get an expression of this type that you'd want to store in a
> unique_ptr<T>.
> 
> So for example:
> char s[] = "abc";
> WrapUnique(&s);
> compiles, even though it's clearly wrong.
> 
> Maybe a multi-dimensional array?
> 
> 2. why is it OK to delete a T(*)[n] through scalar delete? I'm sure I'm
missing
> something obvious here...

OK history of this appears to be along the lines of:

1. I don't think we need it probably.
2. But maybe we can figure out how to write it.
3. In theory someone could make a pointer to an array type and then pass that in
here, though it's not clear how to actually do that on the heap.
4. But we found a way to write it in a static assert.
5. If you hit this static assert things wouldn't compile anyways, but maybe this
is nicer?

This isn't buying much of anything but *looks* like it's preventing arrays which
it is *not*. It's preventing pointer to an array type of unknown bounds.

I think we should not reproduce this static assert, just remove it entirely.

[dcheng, 2015-12-14 17:41:25]

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

https://codereview.chromium.org/1513423002/diff/1/base/memory/ptr_util.h#newc...
base/memory/ptr_util.h:16: static_assert(!std::is_array<T>::value ||
std::extent<T>::value != 0,
On 2015/12/12 at 00:01:22, danakj (behind on reviews) wrote:
> On 2015/12/11 21:32:01, dcheng wrote:
> > After puzzling over this, I still don't understand the reasoning behind
these
> > static asserts. make_scoped_ptr (both in Chromium and in google3) never had
this
> > assert, but the google3 equivalent of this helper has these.
> > 
> > Since this template is specialized on T*, this can only match pointers to
> > arrays.
> > 
> > So T(*)[], T(*)[n], and T(*)[0] (as far as I understand).
> > 
> > 
> > 
> > My rough understanding is that these static asserts are to keep this helper
from
> > returning a unique_ptr<T[]> rather than a unique_ptr<T>.
> > 
> > Two main things I don't understand:
> > 1. how you would get an expression of this type that you'd want to store in
a
> > unique_ptr<T>.
> > 
> > So for example:
> > char s[] = "abc";
> > WrapUnique(&s);
> > compiles, even though it's clearly wrong.
> > 
> > Maybe a multi-dimensional array?
> > 
> > 2. why is it OK to delete a T(*)[n] through scalar delete? I'm sure I'm
missing
> > something obvious here...
> 
> OK history of this appears to be along the lines of:
> 
> 1. I don't think we need it probably.
> 2. But maybe we can figure out how to write it.
> 3. In theory someone could make a pointer to an array type and then pass that
in here, though it's not clear how to actually do that on the heap.
> 4. But we found a way to write it in a static assert.
> 5. If you hit this static assert things wouldn't compile anyways, but maybe
this is nicer?
> 
> This isn't buying much of anything but *looks* like it's preventing arrays
which it is *not*. It's preventing pointer to an array type of unknown bounds.
> 
> I think we should not reproduce this static assert, just remove it entirely.

Done. I've also added some comments warning about the dangers of using this with
an array allocation.

[vmpstr, 2015-12-14 19:05:39]

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

https://codereview.chromium.org/1513423002/diff/1/base/memory/ptr_util.h#newc...
base/memory/ptr_util.h:16: static_assert(!std::is_array<T>::value ||
std::extent<T>::value != 0,
On 2015/12/14 17:41:24, dcheng wrote:
> On 2015/12/12 at 00:01:22, danakj (behind on reviews) wrote:
> > On 2015/12/11 21:32:01, dcheng wrote:
> > > After puzzling over this, I still don't understand the reasoning behind
> these
> > > static asserts. make_scoped_ptr (both in Chromium and in google3) never
had
> this
> > > assert, but the google3 equivalent of this helper has these.
> > > 
> > > Since this template is specialized on T*, this can only match pointers to
> > > arrays.
> > > 
> > > So T(*)[], T(*)[n], and T(*)[0] (as far as I understand).
> > > 
> > > 
> > > 
> > > My rough understanding is that these static asserts are to keep this
helper
> from
> > > returning a unique_ptr<T[]> rather than a unique_ptr<T>.
> > > 
> > > Two main things I don't understand:
> > > 1. how you would get an expression of this type that you'd want to store
in
> a
> > > unique_ptr<T>.
> > > 
> > > So for example:
> > > char s[] = "abc";
> > > WrapUnique(&s);
> > > compiles, even though it's clearly wrong.
> > > 
> > > Maybe a multi-dimensional array?
> > > 
> > > 2. why is it OK to delete a T(*)[n] through scalar delete? I'm sure I'm
> missing
> > > something obvious here...
> > 
> > OK history of this appears to be along the lines of:
> > 
> > 1. I don't think we need it probably.
> > 2. But maybe we can figure out how to write it.
> > 3. In theory someone could make a pointer to an array type and then pass
that
> in here, though it's not clear how to actually do that on the heap.
> > 4. But we found a way to write it in a static assert.
> > 5. If you hit this static assert things wouldn't compile anyways, but maybe
> this is nicer?
> > 
> > This isn't buying much of anything but *looks* like it's preventing arrays
> which it is *not*. It's preventing pointer to an array type of unknown bounds.
> > 
> > I think we should not reproduce this static assert, just remove it entirely.
> 
> Done. I've also added some comments warning about the dangers of using this
with
> an array allocation.

I was nerdsniped by this and I kind of got a test case that I thought would
fail:

// this seems to be illegal, but compilers apparently
// allow this as an extension.
using Array = int[0];
WrapUnique(new Array[1]);

Inside WrapUnique, T is int[0]. However, the static_assert passes because
std::is_array<int[0]>::value is false O_O

In fact

template <typename T> struct deduce_n;
template <typename T, int N> struct deduce_n<T[N]> { enum { value = N }; };

then, deduce_n<int[0]>::value doesn't compile because it says the template is
not defined... deduce_n<int[1]>::value does compile though. I learned something
today: don't use things that the standard says are wrong.

Anyway, I really like your comment instead :)

[danakj, 2015-12-14 19:37:58]

LGTM

[dcheng, 2015-12-14 19:45:03]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2015-12-14 19:46:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1513423002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1513423002/40001

[commit-bot: I haz the power, 2015-12-14 20:07:21]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-12-14 20:08:33]

Description was changed from

==========
Add WrapUnique helper to wrap a raw pointer in a std::unique_ptr.

This is the equivalent of make_scoped_ptr in a post-scoped_ptr world. It
is not called MakeUniquePtr because C++14 adds a std::make_unique helper
with different semantics.

BUG=554522
==========

to

==========
Add WrapUnique helper to wrap a raw pointer in a std::unique_ptr.

This is the equivalent of make_scoped_ptr in a post-scoped_ptr world. It
is not called MakeUniquePtr because C++14 adds a std::make_unique helper
with different semantics.

BUG=554522

Committed: https://crrev.com/2ba07d1f31f04391c83a53a2e49387de58b1ac2d
Cr-Commit-Position: refs/heads/master@{#365079}
==========

[commit-bot: I haz the power, 2015-12-14 20:08:34]

Patchset 3 (id:??) landed as
https://crrev.com/2ba07d1f31f04391c83a53a2e49387de58b1ac2d
Cr-Commit-Position: refs/heads/master@{#365079}