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

base/memory/ptr_util.h

+// Copyright 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef BASE_MEMORY_PTR_UTIL_H_
+#define BASE_MEMORY_PTR_UTIL_H_
+
+#include <memory>
+#include <type_traits>
+
+namespace base {
+
+// Helper function to transfer ownership of a raw pointer to a std::unique_ptr.
+template <typename T>
+std::unique_ptr<T> WrapUnique(T* ptr) {

[danakj, 2015/12/11 22:01:32]

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]

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.

+  static_assert(!std::is_array<T>::value || std::extent<T>::value != 0,

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

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/12 00:01:22]

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:24]

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]

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 :)

+                "cannot wrap a pointer to T[] or T[0]");
+  return std::unique_ptr<T>(ptr);
+}
+
+}  // namespace base
+
+#endif  // BASE_MEMORY_PTR_UTIL_H_