WrapUnique vs. MakeUnique -- do we have a preference?

[James Cook]

I prefer WrapUnique(new Foo), as it means my code continues to have the string "new Foo" in it, which makes it easier for me to find object construction with grep. I also found the MakeUnique<Foo>(params, go, here) syntax a little confusing at first.

OTOH, the comment in base/memory/ptr_util.h says:

// MakeUnique<T>(args) should be preferred over WrapUnique(new T(args)): bare

// calls to `new` should be treated with scrutiny.

Do we have guidance? Both are acceptable? Is it acceptable for a reviewer to require one over the other?

James

[dan...@chromium.org]

base::MakeUnique will eventually become std::make_unique. Its advantages are that it's impossible to hold it wrong AFAIK and it requires less ()s. Its disadvantage is that it requires the constructor to be public so it cant be used in static Create methods (the alternative to WrapUnique is to explicitly use unique_ptr constructors). Whereas WrapUnique can put invalid things into unique_ptr, and it'll compile but ASAN will be sad at you (compile failure for MakeUnique however will come from https://cs.chromium.org/chromium/src/base/memory/ptr_util.h?rcl=0&l=69).

The argument for grepping "new T" is that you expect "new T" to be more common. grepping "MakeUnique<T>" is equally possible. Neither one catches object creation that isn't on the heap.

I am biased since I reviewed that comment but I think it's guidance is appropriate. MakeUnique is safer, and it is where the standard library is going.

(I also don't feel like it's a huge problem if code uses WrapUnique(new T(..)), and probably some rewriter will come and make them all into MakeUnique or make_unique anyway one day.)

[Sylvain Defresne]

I personally think that WrapUnique should disappear and all std::unique_ptr should be create with MakeUnique (or std::make_unique from c++14). This is because MakeUnique is always correct (as it create a new object and wrap it in a std::unique_ptr) while WrapUnique could be used incorrectly (by wrapping a raw pointer owned by another part of the code).

So every time I see a WrapUnique call I have to look carefully at the code to check whether it is a valid call (i.e. WrapUnique(new Foo)) or a potentially invalid call (i.e. WrapUnique(some_ptr.get())). Anything that makes the code harder to read, or increase the risk of mistake should be discouraged at least if we have better safe alternative.

My 0.02$,

-- Sylvain

On Wed, Aug 10, 2016 at 12:42 PM, James Cook <jame...@chromium.org> wrote:

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

[Peter Kasting]

On Wed, Aug 10, 2016 at 12:42 PM, James Cook <jame...@chromium.org> wrote:

I ask authors to use MakeUnique() in the reviews I do.

Dana and Sylvain's comments are applicable. And, as the comment notes, using MakeUnique() to reduce the number of raw "new"s gets us closer to a world in which bare "new" is rare, sticks out to reviewers, and is treated with skepticism.  That, I think would be a world where we had fewer leaked objects.  So I'm hoping to get there someday.

PK

[James Cook]

These seem like good arguments for MakeUnique<>. I will switch to using it. Thanks.

James

[Scott Violet]

I have been trying to use MakeUnique, but it's painful on classes
without a public constructor (as Dana mentions). Could we make it
easier for this to work without having to friend an internal function?

-Scott

[Peter Kasting]

On Wed, Aug 10, 2016 at 1:59 PM, Scott Violet <s...@chromium.org> wrote:

I have been trying to use MakeUnique, but it's painful on classes
without a public constructor (as Dana mentions). Could we make it
easier for this to work without having to friend an internal function?

Does this not work?:

class T {

 public:

  std::unique_ptr<T> Create(...);

 private:

  T(...);

};

std::unique_ptr<T> t = T::Create(...);

That seems like a fine way to deal with classes that want to provide a creation function instead of a direct constructor.

PK

[dan...@chromium.org]

On Wed, Aug 10, 2016 at 1:59 PM, Scott Violet <s...@chromium.org> wrote:

I have been trying to use MakeUnique, but it's painful on classes
without a public constructor (as Dana mentions). Could we make it
easier for this to work without having to friend an internal function?

Without knowing your particular scenario, I've been actively removing static Create() methods that return unique_ptrs and just exposing constructors publicly in src/cc/ lately, which both lets use use MakeUnique, and also lets us make classes direct members of owning class instead of separately-heap-allocated pointers.

Otherwise, I think you'll just have to accept calling "new T" in the method that does create/return a unique_ptr to T. I don't know how else to make MakeUnique work, and I would be against diverging from the future std::make_unique.

[Scott Violet]

That's the pattern I would like to work. Problem is the Create()
function isn't the one calling new, it's MakeUnique, which doesn't
have access to the private constructor.

As to Dana's comment about removing the Create() function and using
the constructor directly, there are cases where Create() does
additional work, may return null or a different implementation. You
can't capture that in a constructor.

-Scott

[Peter Kasting]

On Wed, Aug 10, 2016 at 2:05 PM, Peter Kasting <pkas...@chromium.org> wrote:

Oh, never mind, I see from Dana's reply what you were getting at.  Create() itself still can't call MakeUnique() without making that function a friend of the class.

PK 

[dan...@chromium.org]

On Wed, Aug 10, 2016 at 2:11 PM, Scott Violet <s...@chromium.org> wrote:

That's the pattern I would like to work. Problem is the Create()
function isn't the one calling new, it's MakeUnique, which doesn't
have access to the private constructor.

As to Dana's comment about removing the Create() function and using
the constructor directly, there are cases where Create() does
additional work, may return null or a different implementation. You
can't capture that in a constructor.

Ya you'd need an Init() function instead if you want the constructor public. I've just been converting things in cases where Create always succeeds so far.

[Elliott Sprehn]

On Wed, Aug 10, 2016 at 2:07 PM, <dan...@chromium.org> wrote:

On Wed, Aug 10, 2016 at 1:59 PM, Scott Violet <s...@chromium.org> wrote:

I have been trying to use MakeUnique, but it's painful on classes
without a public constructor (as Dana mentions). Could we make it
easier for this to work without having to friend an internal function?

Without knowing your particular scenario, I've been actively removing static Create() methods that return unique_ptrs and just exposing constructors publicly in src/cc/ lately, which both lets use use MakeUnique, and also lets us make classes direct members of owning class instead of separately-heap-allocated pointers.

That means you can't forward declare the types though right? 

- E 

[dan...@chromium.org]

Yes you can't forward declare direct members. Though as the style guide says, "However, if it would otherwise make sense to use a type as a member by-value, don't convert it to a pointer just to be able to forward-declare the type."

 

- E 

[Raymond Toy]

If MakeUnique is the recommended way, can someone update https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/Source/wtf/UniquePtrTransitionGuide.md with the appropriate recommendations and caveats?  I could write something, but I'm sure to get it wrong since I have to read this every time I change OwnPtr<T>'s.

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev+unsubscribe@chromium.org.

[dan...@chromium.org]

On Thu, Aug 11, 2016 at 3:02 PM, Raymond Toy <rt...@google.com> wrote:

If MakeUnique is the recommended way, can someone update https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/Source/wtf/UniquePtrTransitionGuide.md with the appropriate recommendations and caveats?  I could write something, but I'm sure to get it wrong since I have to read this every time I change OwnPtr<T>'s.

I don't think WTF has a makeUnique equivalent atm. https://cs.chromium.org/chromium/src/third_party/WebKit/Source/wtf/PtrUtil.h?q=wrapUnique&sq=package:chromium&dr=CSs&l=15

[Raymond Toy]

Good point. No updated needed, I guess. (But maybe WTF should have MakeUnique? But that's a different question.)

[Adam Rice]

So is a bulk CL changing WrapUnique(new Foo(...)) to MakeUnique<Foo>(...) wanted?

[Peter Kasting]

On Fri, Aug 12, 2016 at 12:16 AM, Adam Rice <ri...@chromium.org> wrote:

So is a bulk CL changing WrapUnique(new Foo(...)) to MakeUnique<Foo>(...) wanted?

If it compiles, I would personally be in favor of it.  But beware an automated change, since it can run into the problems mentioned already in this thread.

PK

[Adam Rice]

An interesting issue is that NULL doesn't work in MakeUnique(). The type is inferred as long, which then can't be assigned to a pointer type. The obvious solution is to use nullptr.

I think this is probably a unique case as the magic of constant zero is not captured by the C++ type system.

[Christian Biesinger]

Shouldn't you be using nullptr instead of NULL anyway? :)

-Christian

[Joshua Bell]

In a world where we still have private constructors, what is the preferred incantation?

return std::unique_ptr<T>(new T());

return base::WrapUnique(new T());

...?

Also... in a world where 'new' is anathema, how would we go about consing up ref counted types? Is make_refcounted a crazy thought?

[Jeremy Roman]

On Fri, Aug 12, 2016 at 5:16 AM, Adam Rice <ri...@chromium.org> wrote:

An interesting issue is that NULL doesn't work in MakeUnique(). The type is inferred as long, which then can't be assigned to a pointer type. The obvious solution is to use nullptr.

I don't see the issue you're describing around MakeUnique; it should forward all arguments, including NULL, perfectly to the constructor.

If you mean WrapUnique(NULL), writing simply "nullptr" works because it converts to all unique_ptr<T>.

 

I think this is probably a unique case as the magic of constant zero is not captured by the C++ type system.

On 12 August 2016 at 16:20, Peter Kasting <pkas...@chromium.org> wrote:

On Fri, Aug 12, 2016 at 12:16 AM, Adam Rice <ri...@chromium.org> wrote:

So is a bulk CL changing WrapUnique(new Foo(...)) to MakeUnique<Foo>(...) wanted?

If it compiles, I would personally be in favor of it.  But beware an automated change, since it can run into the problems mentioned already in this thread.

PK

[Jeffrey Yasskin]

On Fri, Aug 12, 2016 at 9:38 AM, Joshua Bell <jsb...@chromium.org> wrote:

In a world where we still have private constructors, what is the preferred incantation?

return std::unique_ptr<T>(new T());

return base::WrapUnique(new T());

...?

Also... in a world where 'new' is anathema, how would we go about consing up ref counted types? Is make_refcounted a crazy thought?

std::shared_ptr has a std::make_shared<T>(args...), which is actually more efficient than std::shared_ptr<T>(new T(args...)) (because it does a single allocation that includes the control block). make_refcounted<T>(args...) would, I think, also be slightly more efficient than scoped_refptr<T>(new T(args...)), because it could avoid one refcount operation. (Copy elision would avoid adding those refcounts back as you return the object out of a stack of functions.)

Jeffrey

[dan...@chromium.org]

On Fri, Aug 12, 2016 at 9:38 AM, Joshua Bell <jsb...@chromium.org> wrote:

In a world where we still have private constructors, what is the preferred incantation?

return std::unique_ptr<T>(new T());

return base::WrapUnique(new T());

...?

I'd use WrapUnique unless you like writing out the T twice, they are functionally the same though.

[Adam Rice]

On 13 August 2016 at 03:48, Jeremy Roman <jbr...@chromium.org> wrote:

I don't see the issue you're describing around MakeUnique; it should forward all arguments, including NULL, perfectly to the constructor.

 https://codereview.chromium.org/2246813002/ illustrates the issue. The CL changes

   base::WrapUnique(new SequencedSocketData(NULL, 0, NULL, 0));

to

   base::MakeUnique<SequencedSocketData>(NULL, 0, NULL, 0);

After the change the file no longer compiles.

[Daniel Cheng]

That's because NULL gets type-deduced to long (since it's really just a macro for the integral constant zero), which doesn't convert to a pointer type. We should probably just s/NULL/nullptr/g in the codebase and be done with it.

Daniel

--

[Adam Rice]

Another reason to avoid WrapUnique for Other People's Classes: ADL can mean that it sometimes works without the base:: namespace prefix. This had me scratching my head for a while:

https://cs.chromium.org/chromium/src/chrome/browser/metrics/subprocess_metrics_provider.cc?q=WrapUnique

It's not wrong as such, it's just confusing.

[Peter Kasting]

On Sun, Aug 14, 2016 at 10:58 PM, Daniel Cheng <dch...@chromium.org> wrote:

That's because NULL gets type-deduced to long (since it's really just a macro for the integral constant zero), which doesn't convert to a pointer type. We should probably just s/NULL/nullptr/g in the codebase and be done with it.

I agree with this sentiment.

In practice, a while back a bunch of new external contributors started sending patches to do this, but the patches were mostly created in an automated way, and they broke some things (mostly changing something where the string "NULL" appeared as not-the-pointer-NULL) and we also wanted to prefer "null" to "nullptr" in comments, so we kinda frowned on them.

But a thoughtful such conversion would be good to do.

PK 

[Adam Rice]

CLs replacing a few thousand calls to WrapUnique() with MakeUnique() are now landed or under review.

I want to document what I've learned before I forget it, but I can't find a good place to do so, so I'm going to do it here.

• WrapUnique(new Foo) and WrapUnique(new Foo()) mean something different if Foo does not have a user-defined constructor. I have not rewritten the first type of expression. Determining whether or not Foo has a user-defined constructor requires contextual information. For the sake of future maintainers, please use MakeUnique<Foo>() instead. If you're intentionally leaving off the "()" as an optimisation, please leave a comment.
  
• MakeUnique cannot be used to construct objects whose constructors are protected: or private:. Use WrapUnique() for these. Don't ever friend MakeUnique as this defeats the access control.
  
• The base:: namespace qualifier can be missed off WrapUnique if the object being wrapped is itself in base. I found this surprising. Use MakeUnique instead.
  
• NULL as an argument to MakeUnique doesn't work. Use nullptr instead.
  
• Constants that are passed to MakeUnique must have storage, so that they can be passed by reference. This can lead to linking errors when switching from WrapUnique to MakeUnique.

Future work:

It seems that

  foo = MakeUnique<Foo>();

is better modern C++ style than

  foo.reset(new Foo());

where foo is a unique_ptr<Foo>. It would be useful to rewrite the latter to the former across the codebase, but since it requires knowing the type of "foo" it is non-trivial.

Bonus trivia:

  base::MakeUnique<int>(5) = base::MakeUnique<int>();

compiles and runs.

[Ben]

I want to document what I've learned before I forget it, but I can't find a good place to do so, so I'm going to do it here.

 

I would think one of the style guides would be a good place to capture many of those thoughts ;) 

[Peter Kasting]

On Thu, Aug 25, 2016 at 7:04 AM, Ben <bru...@chromium.org> wrote:

I want to document what I've learned before I forget it, but I can't find a good place to do so, so I'm going to do it here.

 

I would think one of the style guides would be a good place to capture many of those thoughts ;) 

I suggest modifying the C++ Dos and Donts page to add an encouragement to use MakeUnique over bare new or WrapUnique, and then add info from the first two bullets (don't leave off () unless you really mean it, and if so, add a comment; don't friend MakeUnique).

PK

[Wez]

[sort-of off-topic] What _is_ the difference between WrapUnique(new Foo) and WrapUnique(new Foo()), out of interest..?

--

[Peter Kasting]

On Thu, Aug 25, 2016 at 12:41 PM, Wez <w...@chromium.org> wrote:

[sort-of off-topic] What _is_ the difference between WrapUnique(new Foo) and WrapUnique(new Foo()), out of interest..?

WrapUnique() isn't important there.  It's just new Foo vs. new Foo().

More info on that: http://stackoverflow.com/questions/620137/do-the-parentheses-after-the-type-name-make-a-difference-with-new

PK

[Christian Biesinger]

AIUI, if it's a POD type, the latter zero-initializes and the former doesn't.

-Christian

[Peter Kasting]

On Thu, Aug 25, 2016 at 12:46 PM, Christian Biesinger <cbies...@chromium.org> wrote:

AIUI, if it's a POD type, the latter zero-initializes and the former doesn't.

Basically; the issue here is that that's also true for non-POD types with compiler-generated default ctors. but if the ctor is not autogenerated the two are the same.

Unless C++11 changed the rules on this.

PK

[Christian Biesinger]

Yes, reading your link that seems to be true in C++03 but not 98. Interesting. I was not aware of that.  (Presumably 11 is like 03)

-Christian

[dan...@chromium.org]

Here's updated docs: http://en.cppreference.com/w/cpp/language/default_initialization. In c++11 "new T" will be constructed for any class type even if POD. It still applies to basic types that they are not initialized though.

 

-Christian

[Peter Kasting]

Right, this change has relatively little visible effect AFAICT.  In the stackoverflow link, it means that in C++11, struct A is default-initialized when constructed with "new A".  But I believe that has no real effect since default initialization of that struct won't initialize |m|.  I'm not actually sure what the practical difference resulting from this rule change is.

PK

[Adam Rice]

On 26 August 2016 at 04:18, Peter Kasting <pkas...@chromium.org> wrote:

I suggest modifying the C++ Dos and Donts page to add an encouragement to use MakeUnique over bare new or WrapUnique, and then add info from the first two bullets (don't leave off () unless you really mean it, and if so, add a comment; don't friend MakeUnique).

Done: https://sites.google.com/a/chromium.org/dev/developers/coding-style/cpp-dos-and-donts?pli=1#TOC-Prefer-MakeUnique-to-WrapUnique

PTAL.

[Bruce]

> I'm not actually sure what the practical difference resulting from this rule change is.

Pretty huge, when working with types that don't have a user-defined constructor. Here are some concrete examples:

    new char; // Pointer to an uninitialized char

    new char(); // Pointer to a zero initialized char

Allocating a single char seems unlikely, but how about this more plausible scenario:

    new uint8_t[kBufSize]; // Pointer to an uninitialized array of uint8_t

    new uint8_t[kBufSize](); // Pointer to a zero initialized array of uint8_t

If you are going to read data into the array then the first syntax is more efficient, but there are many cases where for correctness you need the second syntax. Note that std::vector does zero initialization, which makes it a (slightly) sub-optimal choice if you are going to immediately read data into its buffer.

These differences apply to any type without a user-defined constructor.

The difference between with and without the () is excruciatingly subtle and not well known so if you are intentionally choosing one or the other for reasons then a comment might be a good idea.

[Peter Kasting]

On Fri, Aug 26, 2016 at 10:55 AM, Bruce <bruce...@chromium.org> wrote:

> I'm not actually sure what the practical difference resulting from this rule change is.

Pretty huge, when working with types that don't have a user-defined constructor. Here are some concrete examples:

    new char; // Pointer to an uninitialized char

    new char(); // Pointer to a zero initialized char

Sure, but it's not the C++11 change that caused that -- that was true as far back as either C++03 or C++98, right?

From http://en.cppreference.com/w/cpp/language/value_initialization , I think the C++11 change here has to do with whether non-POD class types with no user-defined constructor zero their class-type members before calling those members' constructors.

PK

[Primiano Tucci]

Somebody pointed out an internal thread which seem to have some discussion on the same topic. Not sure if it's relevant and makes a difference for the discussion here (which honestly I didn't manage to read it all yet), apologies for the noise if that's the case.

--

[Chris Pickel]

Rather than relying on a comment, wouldn't it be better to have an
alternate function for the purpose? Something along the lines of:

std::unique_ptr<T[]> MakeUninitialized(size_t n) {
static_assert(std::is_pod<T>::value, "non-POD values must be initialized");
return WrapUnique(new T[n]);
}

The main point on the internal thread, for anyone without access, is
essentially "MakeUnique(...) is good because it (correctly) does one
thing, whereas WrapUnique(p) can wrap new allocations, assume
ownership of unowned pointers, or (doubly, incorrectly) assume
ownership of owned pointers."

Similarly, MakeUninitialized() would do one thing and invite
appropriate scrutiny.

[Nico Weber]

I might be alone here, and I'm late to the party, but I still don't quite see the point of MakeUnique(). If you write C++, you'll have to understand new, and WrapUnique(new Foo(...)) is easy to understand. As this thread shows, MakeUnique() has a pretty long list of asterisks attached to it, without a benefit other than "no bare new" ¯\_(ツ)_/¯

[Peter Kasting]

On Thu, Sep 1, 2016 at 8:40 AM, Nico Weber <tha...@chromium.org> wrote:

I might be alone here, and I'm late to the party, but I still don't quite see the point of MakeUnique(). If you write C++, you'll have to understand new, and WrapUnique(new Foo(...)) is easy to understand. As this thread shows, MakeUnique() has a pretty long list of asterisks attached to it, without a benefit other than "no bare new" ¯\_(ツ)_/¯

Did you see Titus' email in the linked internal thread?  It sketches out an argument.

PK

[Primiano Tucci]

Just stumbled in an odd thing about WrapUnique while writing some code today. It seems that WrapUnique wraps to the wrong type (and hence uses the wrong deleter) for unique_ptr<array[]>. Example:

size_t N = 4;

std::unique_ptr<uint8_t[]> foo(new uint8_t[N]);  // This is what I intend to do

std::unique_ptr<uint8_t[]> bar = base::WrapUnique(new uint8_t[N]);  // This fails

error: no viable conversion from 'unique_ptr<unsigned char>' to 'unique_ptr<uint8_t []>'

  std::unique_ptr<uint8_t[]> bar = base::WrapUnique(new uint8_t[N]);

                             ^     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

/Users/primiano/code/chrome/src/third_party/llvm-build/Release+Asserts/bin/../include/c++/v1/memory:2779:29: note: candidate constructor (the implicit copy constructor) not viable: no known conversion from 'std::unique_ptr<unsigned char>' to 'const std::__1::unique_ptr<unsigned char [], std::__1::default_delete<unsigned char []> > &' for 1st argument

class _LIBCPP_TYPE_VIS_ONLY unique_ptr<_Tp[], _Dp>

Note that if I did use *auto* bar this would have silently compiled, but ended up using the wrong deleter (default_deleter<uint8_t>  instead of default_deleter<uint8_t[]>, which should translate into delete instead of delete []). 

Probably using the wrong deleter works anyways in most implementations (i.e. as long as "operator new" and "operator new[]" both end up calling malloc()) but IMHO is a bit disturbing. The implementation of the global operator new[] could use a different heap. In such a case, using the wrong deleter would cause a crash.

instead

std::unique_ptr<uint8_t[]> baz = base::MakeUnique<uint8_t[]>(N);

compiles fine (at least on mac) and hence I think that

auto baz =  base::MakeUnique<uint8_t[]>(N)

would have done the right thing

[Daniel Cheng]

Yes, it's unsafe to use WrapUnique with something allocated for operator new[]. However, WrapUnique can't distinguish between these cases because the return value is a pointer for non-array new and new[].

Daniel

[Primiano Tucci]

Right. But it this seems to me an argument for not using that if I have to remember that arrays are dangerous there.

Whatever works for me, regardless of verbosity, as long as I have to teach my brain just one thing.

[Evan Stade]

On Wednesday, August 24, 2016 at 4:06:12 AM UTC-6, Adam Rice wrote:

CLs replacing a few thousand calls to WrapUnique() with MakeUnique() are now landed or under review.

I want to document what I've learned before I forget it, but I can't find a good place to do so, so I'm going to do it here.

• WrapUnique(new Foo) and WrapUnique(new Foo()) mean something different if Foo does not have a user-defined constructor. I have not rewritten the first type of expression. Determining whether or not Foo has a user-defined constructor requires contextual information. For the sake of future maintainers, please use MakeUnique<Foo>() instead. If you're intentionally leaving off the "()" as an optimisation, please leave a comment.
  
• MakeUnique cannot be used to construct objects whose constructors are protected: or private:. Use WrapUnique() for these. Don't ever friend MakeUnique as this defeats the access control.
  
• The base:: namespace qualifier can be missed off WrapUnique if the object being wrapped is itself in base. I found this surprising. Use MakeUnique instead.
  
• NULL as an argument to MakeUnique doesn't work. Use nullptr instead.
  
• Constants that are passed to MakeUnique must have storage, so that they can be passed by reference. This can lead to linking errors when switching from WrapUnique to MakeUnique.

Future work:

It seems that

  foo = MakeUnique<Foo>();

is better modern C++ style than

  foo.reset(new Foo());

The above came up in a code review today. Is this agreed on? It didn't seem to make it into the C++ do's and don'ts which only touches on WrapUnique. The other place this comes up is in an initializer list:

  : foo_ptr_(new Foo()),

vs.

  : foo_ptr_(base::MakeUnique<Foo>()),

Is there some reason to favor the latter?

[Jeremy Roman]

On Tue, Aug 29, 2017 at 5:18 PM, Evan Stade <est...@chromium.org> wrote:

On Wednesday, August 24, 2016 at 4:06:12 AM UTC-6, Adam Rice wrote:

CLs replacing a few thousand calls to WrapUnique() with MakeUnique() are now landed or under review.

I want to document what I've learned before I forget it, but I can't find a good place to do so, so I'm going to do it here.

• WrapUnique(new Foo) and WrapUnique(new Foo()) mean something different if Foo does not have a user-defined constructor. I have not rewritten the first type of expression. Determining whether or not Foo has a user-defined constructor requires contextual information. For the sake of future maintainers, please use MakeUnique<Foo>() instead. If you're intentionally leaving off the "()" as an optimisation, please leave a comment.
  
• MakeUnique cannot be used to construct objects whose constructors are protected: or private:. Use WrapUnique() for these. Don't ever friend MakeUnique as this defeats the access control.
  
• The base:: namespace qualifier can be missed off WrapUnique if the object being wrapped is itself in base. I found this surprising. Use MakeUnique instead.
  
• NULL as an argument to MakeUnique doesn't work. Use nullptr instead.
  
• Constants that are passed to MakeUnique must have storage, so that they can be passed by reference. This can lead to linking errors when switching from WrapUnique to MakeUnique.

Future work:

It seems that

  foo = MakeUnique<Foo>();

is better modern C++ style than

  foo.reset(new Foo());

The above came up in a code review today. Is this agreed on? It didn't seem to make it into the C++ do's and don'ts which only touches on WrapUnique. The other place this comes up is in an initializer list:

  : foo_ptr_(new Foo()),

vs.

  : foo_ptr_(base::MakeUnique<Foo>()),

Is there some reason to favor the latter?

I prefer the latter (or now, std::make_unique), because it makes it clear that the created pointer will be uniquely owned (without having to check foo_ptr_'s type), and it reinforces the habit that using raw "new" (except for Oilpan types) is a red flag.

[Peter Kasting]

On Tue, Aug 29, 2017 at 2:18 PM, Evan Stade <est...@chromium.org> wrote:

On Wednesday, August 24, 2016 at 4:06:12 AM UTC-6, Adam Rice wrote:

It seems that

  foo = MakeUnique<Foo>();

is better modern C++ style than

  foo.reset(new Foo());

The above came up in a code review today. Is this agreed on? It didn't seem to make it into the C++ do's and don'ts which only touches on WrapUnique. The other place this comes up is in an initializer list:

  : foo_ptr_(new Foo()),

vs.

  : foo_ptr_(base::MakeUnique<Foo>()),

Is there some reason to favor the latter?

The relevant line is "In general, bare calls to "new" require careful scrutiny. Bare calls to "new" are currently required to construct reference-counted types; however, reference counted types themselves require careful scrutiny."  This is broader than just WrapUnique, even if that's the name of the larger section.

For the reasons given earlier in this thread/the internal style threads, I've been asking authors to avoid bare new whenever possible, including in both the cases you mentioned.

PK

[Jean-François Geyelin]

The above came up in a code review today. Is this agreed on? It didn't seem to make it into the C++ do's and don'ts which only touches on WrapUnique. The other place this comes up is in an initializer list:

  : foo_ptr_(new Foo()),

vs.

  : foo_ptr_(base::MakeUnique<Foo>()),

Is there some reason to favor the latter?

If foo_ptr_'s type changes to Foo* the latter will create a compiler error, while the former will happily compile (and add a leak).

[Charles Harrison]

We shouldn't need to use bare new for ref counted types, now that we have base::MakeRefCounted, right?

--

--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---

You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev+unsubscribe@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/19ad994c-2869-4234-a0ba-ab2a6937bf6c%40chromium.org.