Add base::MakeUnique to mirror C++14's std::make_unique.

Add base::MakeUnique to mirror C++14's std::make_unique.

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

[dcheng, 2015-12-15 23:40:16]

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

[Ryan Sleevi, 2015-12-15 23:53:41]

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

https://codereview.chromium.org/1532433002/diff/1/base/memory/ptr_util.h#newc...
base/memory/ptr_util.h:44: // calls to `new` should be treated with scrutiny.
Is this level of warning needed?

That is, it seems like it'd be a common pattern for factory-creation where
"CreateOrFail" is a thing, where the ctor is private but a unique_ptr<> is
returned.

class Foo {
 public:
  static std::unique_ptr<Foo> CreateFooWithA(...);
  static std::unique_ptr<Foo> CreateFooWithB(...);

 private:
  Foo();
  bool Init(...);
};

Or is this pattern proscribed, in favour of

class Foo {
 public:
  Foo(WithA, ...);
  Foo(WithB, ...);

  bool Init(...);
};

?

[dcheng, 2015-12-15 23:59:50]

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

https://codereview.chromium.org/1532433002/diff/1/base/memory/ptr_util.h#newc...
base/memory/ptr_util.h:44: // calls to `new` should be treated with scrutiny.
On 2015/12/15 at 23:53:40, Ryan Sleevi wrote:
> Is this level of warning needed?
> 
> That is, it seems like it'd be a common pattern for factory-creation where
"CreateOrFail" is a thing, where the ctor is private but a unique_ptr<> is
returned.
> 
> class Foo {
>  public:
>   static std::unique_ptr<Foo> CreateFooWithA(...);
>   static std::unique_ptr<Foo> CreateFooWithB(...);
> 
>  private:
>   Foo();
>   bool Init(...);
> };
> 
> Or is this pattern proscribed, in favour of
> 
> class Foo {
>  public:
>   Foo(WithA, ...);
>   Foo(WithB, ...);
> 
>   bool Init(...);
> };
> 
> ?

It seems like you could still get the former pattern if you're willing to friend
MakeUnique... in some sense, it seems similar to how we force protected/private
destructors for refcounted types, which then have to friend RefCounted.

[Jeffrey Yasskin, 2015-12-16 00:02:13]

On 2015/12/15 23:59:50, dcheng wrote:
> https://codereview.chromium.org/1532433002/diff/1/base/memory/ptr_util.h
> File base/memory/ptr_util.h (right):
> 
>
https://codereview.chromium.org/1532433002/diff/1/base/memory/ptr_util.h#newc...
> base/memory/ptr_util.h:44: // calls to `new` should be treated with scrutiny.
> On 2015/12/15 at 23:53:40, Ryan Sleevi wrote:
> > Is this level of warning needed?
> > 
> > That is, it seems like it'd be a common pattern for factory-creation where
> "CreateOrFail" is a thing, where the ctor is private but a unique_ptr<> is
> returned.
> > 
> > class Foo {
> >  public:
> >   static std::unique_ptr<Foo> CreateFooWithA(...);
> >   static std::unique_ptr<Foo> CreateFooWithB(...);
> > 
> >  private:
> >   Foo();
> >   bool Init(...);
> > };
> > 
> > Or is this pattern proscribed, in favour of
> > 
> > class Foo {
> >  public:
> >   Foo(WithA, ...);
> >   Foo(WithB, ...);
> > 
> >   bool Init(...);
> > };
> > 
> > ?
> 
> It seems like you could still get the former pattern if you're willing to
friend
> MakeUnique... in some sense, it seems similar to how we force
protected/private
> destructors for refcounted types, which then have to friend RefCounted.

Friend'ing MakeUnique defeats the purpose of having a public create method with
private constructors, since anyone can call MakeUnique directly. :)

[dcheng, 2015-12-17 18:42:26]

On 2015/12/16 at 00:02:13, jyasskin wrote:
> On 2015/12/15 23:59:50, dcheng wrote:
> > https://codereview.chromium.org/1532433002/diff/1/base/memory/ptr_util.h
> > File base/memory/ptr_util.h (right):
> > 
> >
https://codereview.chromium.org/1532433002/diff/1/base/memory/ptr_util.h#newc...
> > base/memory/ptr_util.h:44: // calls to `new` should be treated with
scrutiny.
> > On 2015/12/15 at 23:53:40, Ryan Sleevi wrote:
> > > Is this level of warning needed?
> > > 
> > > That is, it seems like it'd be a common pattern for factory-creation where
> > "CreateOrFail" is a thing, where the ctor is private but a unique_ptr<> is
> > returned.
> > > 
> > > class Foo {
> > >  public:
> > >   static std::unique_ptr<Foo> CreateFooWithA(...);
> > >   static std::unique_ptr<Foo> CreateFooWithB(...);
> > > 
> > >  private:
> > >   Foo();
> > >   bool Init(...);
> > > };
> > > 
> > > Or is this pattern proscribed, in favour of
> > > 
> > > class Foo {
> > >  public:
> > >   Foo(WithA, ...);
> > >   Foo(WithB, ...);
> > > 
> > >   bool Init(...);
> > > };
> > > 
> > > ?
> > 
> > It seems like you could still get the former pattern if you're willing to
friend
> > MakeUnique... in some sense, it seems similar to how we force
protected/private
> > destructors for refcounted types, which then have to friend RefCounted.
> 
> Friend'ing MakeUnique defeats the purpose of having a public create method
with private constructors, since anyone can call MakeUnique directly. :)

Doh, right. So what I'd say is that the factory Create() pattern with a private
ctor is one place it'd make sense to use WrapUnique over MakeUnique. However, it
seems to me that make_scoped_ptr(new T) without a private ctor is much more
common (~1413 instances, most which don't call a private ctor), and that's
exactly the situation we should encourage use of MakeUnique. In light of that, I
think this recommendation makes sense: use MakeUnique when possible, and only
fall back to WrapUnique when MakeUnique doesn't work for some reason.

[Ryan Sleevi, 2015-12-17 18:45:23]

On 2015/12/17 18:42:26, dcheng wrote:
> Doh, right. So what I'd say is that the factory Create() pattern with a
private
> ctor is one place it'd make sense to use WrapUnique over MakeUnique. However,
it
> seems to me that make_scoped_ptr(new T) without a private ctor is much more
> common (~1413 instances, most which don't call a private ctor), and that's
> exactly the situation we should encourage use of MakeUnique. In light of that,
I
> think this recommendation makes sense: use MakeUnique when possible, and only
> fall back to WrapUnique when MakeUnique doesn't work for some reason.

Right, I'd totally buy that recommendation in the comment; I think the "treat
with scrutiny" may be a bit too pejorative/implying there's a bug, when really
there's a preferred path and, when that doesn't work, an alternate path

[Nico, 2015-12-17 18:49:55]

What's the motivation for adding this at this point?

[dcheng, 2015-12-17 19:01:51]

On 2015/12/17 at 18:49:55, thakis wrote:
> What's the motivation for adding this at this point?

To encourage people to use std::unique_ptr whenever possible instead of raw
pointers.
It's also standardized in C++14, and I'm assuming we'll eventually move to
C++14. Encouraging this pattern before we get C++14 is similar to how we used
scoped_ptr for many years before std::unique_ptr.

[Nico, 2015-12-17 19:06:24]

On 2015/12/17 19:01:51, dcheng wrote:
> On 2015/12/17 at 18:49:55, thakis wrote:
> > What's the motivation for adding this at this point?
> 
> To encourage people to use std::unique_ptr whenever possible instead of raw
> pointers.

That seems to be going fairly well already:
https://code.google.com/p/chromium/codesearch#search/&q=scoped_ptr&sq=package...
(scoped_ptr and unique_ptr are basically the same thing)

> It's also standardized in C++14, and I'm assuming we'll eventually move to
> C++14. Encouraging this pattern before we get C++14 is similar to how we used
> scoped_ptr for many years before std::unique_ptr.

Does the pattern solve a real problem though? It makes allocations look
different yet again and is probably surprising to people not used to forwarding
functions (i.e. almost everyone), nobody else has adopted c++14 widely, it's
named like make_pair but behaves widely different, etc.

[danakj, 2015-12-17 19:45:32]

On Thu, Dec 17, 2015 at 11:06 AM, <thakis@chromium.org> wrote:

> On 2015/12/17 19:01:51, dcheng wrote:
>
>> On 2015/12/17 at 18:49:55, thakis wrote:
>> > What's the motivation for adding this at this point?
>>
>
> To encourage people to use std::unique_ptr whenever possible instead of raw
>> pointers.
>>
>
> That seems to be going fairly well already:
>
>
https://code.google.com/p/chromium/codesearch#search/&q=scoped_ptr&sq=package...
> (scoped_ptr and unique_ptr are basically the same thing)


I think it's to help us move to std::unique_ptr instead of scoped_ptr (ie
to delete scoped_ptr).


>
> It's also standardized in C++14, and I'm assuming we'll eventually move to
>> C++14. Encouraging this pattern before we get C++14 is similar to how we
>> used
>> scoped_ptr for many years before std::unique_ptr.
>>
>
> Does the pattern solve a real problem though? It makes allocations look
> different yet again and is probably surprising to people not used to
> forwarding
> functions (i.e. almost everyone), nobody else has adopted c++14 widely,
> it's
> named like make_pair but behaves widely different, etc.
>

When we do get C++14, I think that we're going to want to rewrite
allocation sites as std::make_unique, and that'd be much easier if people
are already using base::MakeUnique.

AIUI, the problem it solves is that WrapUnique does the wrong thing for
arrays, whereas MakeUnique doesn't.

Forwarding functions exist in a bunch of other places already such as
emplace_back, they are still new to our code base, but I don't think people
can't figure them out (they're much easier to use/learn than other stuff,
and the compiler will simply tell you if you're doing it wrong ya?).

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

[danakj, 2016-03-14 20:30:29]

bumpyity bump. I'd really like to see people writing MakeUnique(foo, bar)
instead of WrapUnique(new Thing(foo, bar)) in the future, and it's now a
question of what do we change calls of make_scoped_ptr to as we are able to use
unique_ptr now.

Nico, are you any more on board with this now?

[esprehn, 2016-04-01 03:38:11]

On 2016/03/14 at 20:30:29, danakj wrote:
> bumpyity bump. I'd really like to see people writing MakeUnique(foo, bar)
instead of WrapUnique(new Thing(foo, bar)) in the future, and it's now a
question of what do we change calls of make_scoped_ptr to as we are able to use
unique_ptr now.
> 
> Nico, are you any more on board with this now?

Does code search understand this as a site that invokes the constructor? Today I
can find all usage easily.

[dcheng, 2016-04-01 03:44:24]

On 2016/04/01 at 03:38:11, esprehn wrote:
> On 2016/03/14 at 20:30:29, danakj wrote:
> > bumpyity bump. I'd really like to see people writing MakeUnique(foo, bar)
instead of WrapUnique(new Thing(foo, bar)) in the future, and it's now a
question of what do we change calls of make_scoped_ptr to as we are able to use
unique_ptr now.
> > 
> > Nico, are you any more on board with this now?
> 
> Does code search understand this as a site that invokes the constructor? Today
I can find all usage easily.

Internal codesearch seems to be able to. Example: https://goto.google.com/modhg

[Peter Kasting, 2016-04-01 05:06:32]

FWIW, "Effective Modern C++" item 21 deals with motivations for using the
make_xxx() family of functions.  To me, the most compelling argument it makes is
an exception-safety-based one, which isn't relevant for us.  However, there are
also potential verbosity reductions, and, if we at some point allow shared_ptr,
efficiency improvements.  This also makes it a bit easier to get to a world
where we minimize the use of raw "new", and even possibly start to audit for it
at some point (I would be happy to have a rule that raw "new" is banned except
when you need to do something you can't do any other way).

I think this is more a question of "when" than "if"; I can't see us rejecting
use of make_xxx() forever, and if not, then what do we lose by allowing
beginning that transition now?  Better to start easing people into new patterns
sooner than later IMO.  So I support adding this.

[dcheng, 2016-04-19 16:25:59]

Ping =)

[Nico, 2016-04-20 20:44:29]

If Dana thinks we should do this, then go ahead. It's in the "I can't believe
they added this to the language" corner in my head, but I suppose it's a done
deed.

[jbroman, 2016-04-23 03:28:09]

On 2016/04/20 at 20:44:29, thakis wrote:
> If Dana thinks we should do this, then go ahead. It's in the "I can't believe
they added this to the language" corner in my head, but I suppose it's a done
deed.

I'm of two minds.

On the one hand, both the standard and Google internal are moving toward
make_unique. That probably suggests this is inevitable at some point.

On the other hand, we don't really benefit from the exception safety, and I have
some ease-of-use concerns:
- makes finding constructor call sites in code search harder
- currently less familiar
- unfortunate effects for visibility (public/protected/private gets a little
funny when you're indirecting through make_unique)

So on the whole I'm solidly in neutral territory.

[dcheng, 2016-04-25 23:27:14]

On 2016/04/23 at 03:28:09, jbroman wrote:
> On 2016/04/20 at 20:44:29, thakis wrote:
> > If Dana thinks we should do this, then go ahead. It's in the "I can't
believe they added this to the language" corner in my head, but I suppose it's a
done deed.
> 
> I'm of two minds.
> 
> On the one hand, both the standard and Google internal are moving toward
make_unique. That probably suggests this is inevitable at some point.
> 
> On the other hand, we don't really benefit from the exception safety, and I
have some ease-of-use concerns:
> - makes finding constructor call sites in code search harder

Can you explain what you mean by this? Internal codesearch seems to be able to
handle this (I'm happy to link to some examples)

> - currently less familiar
> - unfortunate effects for visibility (public/protected/private gets a little
funny when you're indirecting through make_unique)
> 
> So on the whole I'm solidly in neutral territory.

[jbroman, 2016-05-03 21:32:06]

On 2016/04/25 at 23:27:14, dcheng wrote:
> On 2016/04/23 at 03:28:09, jbroman wrote:
> > On 2016/04/20 at 20:44:29, thakis wrote:
> > > If Dana thinks we should do this, then go ahead. It's in the "I can't
believe they added this to the language" corner in my head, but I suppose it's a
done deed.
> > 
> > I'm of two minds.
> > 
> > On the one hand, both the standard and Google internal are moving toward
make_unique. That probably suggests this is inevitable at some point.
> > 
> > On the other hand, we don't really benefit from the exception safety, and I
have some ease-of-use concerns:
> > - makes finding constructor call sites in code search harder
> 
> Can you explain what you mean by this? Internal codesearch seems to be able to
handle this (I'm happy to link to some examples)

Discussed offline; the internal code search feature is fairly restricted at
present. If this were fixed so that xrefs would work through a base::MakeUnique,
I'd be much happier.

> > - currently less familiar
> > - unfortunate effects for visibility (public/protected/private gets a little
funny when you're indirecting through make_unique)
> > 
> > So on the whole I'm solidly in neutral territory.

[dcheng, 2016-05-04 16:49:47]

On 2016/05/03 at 21:32:06, jbroman wrote:
> On 2016/04/25 at 23:27:14, dcheng wrote:
> > On 2016/04/23 at 03:28:09, jbroman wrote:
> > > On 2016/04/20 at 20:44:29, thakis wrote:
> > > > If Dana thinks we should do this, then go ahead. It's in the "I can't
believe they added this to the language" corner in my head, but I suppose it's a
done deed.
> > > 
> > > I'm of two minds.
> > > 
> > > On the one hand, both the standard and Google internal are moving toward
make_unique. That probably suggests this is inevitable at some point.
> > > 
> > > On the other hand, we don't really benefit from the exception safety, and
I have some ease-of-use concerns:
> > > - makes finding constructor call sites in code search harder
> > 
> > Can you explain what you mean by this? Internal codesearch seems to be able
to handle this (I'm happy to link to some examples)
> 
> Discussed offline; the internal code search feature is fairly restricted at
present. If this were fixed so that xrefs would work through a base::MakeUnique,
I'd be much happier.

http://cl/121438905 is going to land shortly. Not sure how/when codesearch rolls
out new releases though.

> 
> > > - currently less familiar
> > > - unfortunate effects for visibility (public/protected/private gets a
little funny when you're indirecting through make_unique)
> > > 
> > > So on the whole I'm solidly in neutral territory.

[dcheng, 2016-05-10 04:01:01]

Can I get an official LGTM? I've landed the CL to make codesearch look through
MakeUnique calls, and having this CL landed should help us know when it goes
live.

[danakj, 2016-05-10 20:56:51]

LGTM

[dcheng, 2016-05-10 20:59:42]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2016-05-10 21:00:22]

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

[commit-bot: I haz the power, 2016-05-10 22:21:48]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-05-10 22:24:41]

Description was changed from

==========
Add base::MakeUnique to mirror C++14's std::make_unique.

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

to

==========
Add base::MakeUnique to mirror C++14's std::make_unique.

BUG=554522

Committed: https://crrev.com/c22f00f4194a50548a81a2caed975cbdc340c75a
Cr-Commit-Position: refs/heads/master@{#392744}
==========

[commit-bot: I haz the power, 2016-05-10 22:24:43]

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