Introducing mozilla::UniquePtr and mozilla::MakeUnique for |new T| and |new T[]| resources

[Jeff Walden]

Hey all,

mfbt recently picked up mozilla::UniquePtr<T or T[]>, a smart pointer for objects created using |new| or |new[]|. UniquePtr will automatically |delete| or |delete[]| its pointer when it's destroyed. (You can also define custom deletion policies if you want.)

Additionally, you can safely embed UniquePtr in properly-written container classes. This is dangerous or impossible with the current alternatives, because they consider the act of copying to *move* the pointer out of the source object (mutating it), into the sink object.

mfbt also gained MakeUnique, which works like UniquePtr(new T(...)) or UniquePtr(new T[n]()). With MakeUnique there's no point at which it's possible for the new object to be forgotten, leaked, double-deleted, etc. without mistakes elsewhere. I recommend considering MakeUnique calls as idiomatic, such that the pointer type is obvious and isn't repeated twice. If you do this, |auto| nicely simplifies such declarations:

auto memoryPage = MakeUnique<uint8_t[]>(4096); // 4096-byte array
auto i = MakeUnique<int8_t>(17); // new int(17)

More details and examples of how to use UniquePtr and MakeUnique are here:

http://whereswalden.com/2014/07/31/mfbt-now-has-uniqueptr-and-makeunique-for-managing-singly-owned-resources/

As always, the mfbt header itself includes copious documentation and examples of how to use it:

http://mxr.mozilla.org/mozilla-central/source/mfbt/UniquePtr.h

One last note. UniquePtr is patterned on std::unique_ptr, the C++11 standard version of this idiom. The current mozilla::Scoped class is another recent attempt to address largely the same needs. The older nsAutoPtr and nsAutoArrayPtr classes are even older attempts at a solution, that suffer from the (T&) non-copy-constructor problem mentioned before. Scoped, nsAutoPtr, and nsAutoArrayPtr shouldn't be used now that UniquePtr is available, and the former class is now deprecated (and is slowly being removed). (I consider nsAutoPtr and nsAutoArrayPtr deprecated as well, but it's not within my power to authoritatively declare it so. Nonetheless, you should treat them as such.)

Jeff

[Robert O'Callahan]

On Fri, Aug 1, 2014 at 11:24 AM, Jeff Walden <jwald...@mit.edu> wrote:

> One last note. UniquePtr is patterned on std::unique_ptr, the C++11
> standard version of this idiom. The current mozilla::Scoped class is
> another recent attempt to address largely the same needs. The older
> nsAutoPtr and nsAutoArrayPtr classes are even older attempts at a solution,
> that suffer from the (T&) non-copy-constructor problem mentioned before.
> Scoped, nsAutoPtr, and nsAutoArrayPtr shouldn't be used now that UniquePtr
> is available, and the former class is now deprecated (and is slowly being
> removed). (I consider nsAutoPtr and nsAutoArrayPtr deprecated as well, but
> it's not within my power to authoritatively declare it so. Nonetheless,
> you should treat them as such.)
>

Is there anything blocking us from mass-removing nsAutoPtr/nsAutoArrayPtr
in favour of UniquePtr?

I think it's more important to remove old stuff than add new stuff, so the
sooner we can mass-remove nsAutoPtr/nsAutoArrayPtr the better.

Rob
--
oIo otoeololo oyooouo otohoaoto oaonoyooonoeo owohooo oioso oaonogoroyo
owoiotoho oao oboroootohoeoro oooro osoiosotoeoro owoiololo oboeo
osouobojoeocoto otooo ojouodogomoeonoto.o oAogoaoiono,o oaonoyooonoeo
owohooo
osoaoyoso otooo oao oboroootohoeoro oooro osoiosotoeoro,o o‘oRoaocoao,o’o
oioso
oaonosowoeoroaoboloeo otooo otohoeo ocooouoroto.o oAonodo oaonoyooonoeo
owohooo
osoaoyoso,o o‘oYooouo ofooooolo!o’o owoiololo oboeo oiono odoaonogoeoro
ooofo
otohoeo ofoioroeo ooofo ohoeololo.

[Trevor Saunders]

On Fri, Aug 01, 2014 at 11:52:03AM +1200, Robert O'Callahan wrote:
> On Fri, Aug 1, 2014 at 11:24 AM, Jeff Walden <jwald...@mit.edu> wrote:
>

> > One last note. UniquePtr is patterned on std::unique_ptr, the C++11
> > standard version of this idiom. The current mozilla::Scoped class is
> > another recent attempt to address largely the same needs. The older
> > nsAutoPtr and nsAutoArrayPtr classes are even older attempts at a solution,
> > that suffer from the (T&) non-copy-constructor problem mentioned before.

nsAutoPtr is kind of half fixed at this point fwiw.

> > Scoped, nsAutoPtr, and nsAutoArrayPtr shouldn't be used now that UniquePtr
> > is available, and the former class is now deprecated (and is slowly being
> > removed). (I consider nsAutoPtr and nsAutoArrayPtr deprecated as well, but
> > it's not within my power to authoritatively declare it so. Nonetheless,
> > you should treat them as such.)
> >
>

> Is there anything blocking us from mass-removing nsAutoPtr/nsAutoArrayPtr
> in favour of UniquePtr?

Well, there's at least one place in the tree that uses nsAutoPtr in a
broken way (ExplicitChildIterator which I have a patch for) but the
posibility of that kind of issue makes it not totally trivial.

> I think it's more important to remove old stuff than add new stuff, so the
> sooner we can mass-remove nsAutoPtr/nsAutoArrayPtr the better.

For nsAutoPtr I suspect the easiest thing to do is first finish fixing
usage of its weird copy ctor, at which point it should be trivial to
remove it.

Trev

>
> Rob
> --
> oIo otoeololo oyooouo otohoaoto oaonoyooonoeo owohooo oioso oaonogoroyo
> owoiotoho oao oboroootohoeoro oooro osoiosotoeoro owoiololo oboeo
> osouobojoeocoto otooo ojouodogomoeonoto.o oAogoaoiono,o oaonoyooonoeo
> owohooo
> osoaoyoso otooo oao oboroootohoeoro oooro osoiosotoeoro,o o‘oRoaocoao,o’o
> oioso
> oaonosowoeoroaoboloeo otooo otohoeo ocooouoroto.o oAonodo oaonoyooonoeo
> owohooo
> osoaoyoso,o o‘oYooouo ofooooolo!o’o owoiololo oboeo oiono odoaonogoeoro
> ooofo
> otohoeo ofoioroeo ooofo ohoeololo.

> _______________________________________________
> dev-platform mailing list
> dev-pl...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform

[Jeff Walden]

On 07/31/2014 04:52 PM, Robert O'Callahan wrote:
> Is there anything blocking us from mass-removing nsAutoPtr/nsAutoArrayPtr
> in favour of UniquePtr?

I'm not unsure what the state of mass-rewriting tools are these days, to start. But assuming they're up to snuff, there might or might not be. There are a variety of little differences that rewriting would have to carefully consider:

* nsAutoPtr<T> implicitly converts to T*, UniquePtr requires get()
* nsAutoPtr<T> has one of the bad (T&) constructors that's probably in ubiquitous use (and those can't safely be changed except manually, unless you want to pretend all existing code is correct)
* nsAutoPtr can be assigned T*, UniquePtr can only be assigned nullptr or a temporary/moved UniquePtr

And probably more beyond that, I was just skimming.

Additionally, UniquePtr is best used by threading it through interfaces, beyond just immediate use sites. Rather than returning a raw pointer, code should return UniquePtr instead. Rather than extracting a raw pointer to pass to a method, code should pass Move(ptr) instead, and that argument should be a UniquePtr&&. These changes can't be mechanized.

> I think it's more important to remove old stuff than add new stuff, so the
> sooner we can mass-remove nsAutoPtr/nsAutoArrayPtr the better.

In principle, perhaps. But JS wanted UniquePtr as much as gfx (among others, I believe) did, and nsAutoPtr wouldn't cut it there, so it had to happen anyway.

Jeff

[Neil]

Jeff Walden wrote:

>Additionally, UniquePtr is best used by threading it through interfaces, beyond just immediate use sites. Rather than returning a raw pointer, code should return UniquePtr instead. Rather than extracting a raw pointer to pass to a method, code should pass Move(ptr) instead, and that argument should be a UniquePtr&&.
>

Only UniquePtr's own copy and assignment operators should take
UniquePtr&&. Other call sites should either take const UniquePtr& (if
they will not take ownership of the pointer), UniquePtr& (if they may or
may not need to take ownership of the pointer) or UniquePtr (if they
will take ownership of the pointer).

--
Warning: May contain traces of nuts.

[Jeff Walden]

On 08/01/2014 03:48 AM, Neil wrote:
> Only UniquePtr's own copy and assignment operators should take UniquePtr&&. Other call sites should either take const UniquePtr& (if they will not take ownership of the pointer), UniquePtr& (if they may or may not need to take ownership of the pointer) or UniquePtr (if they will take ownership of the pointer).

Oops, you're largely right. There are perhaps rare cases where it might make sense to accept UniquePtr&&, but it's pretty uncommon. I somewhat meant to elaborate on this in the blog post, then forgot. Probably I won't bother at this point.

But, largely what I would have said is encapsulated in this Stack Overflow question/answer:

http://stackoverflow.com/questions/8114276/how-do-i-pass-a-unique-ptr-argument-to-a-constructor-or-a-function

This discussion is much less intricate and technical than the rvalue reference series I linked in the post as "subtle", so it's worth a read if you're interested and won't take up that much time to digest. At the same time, it's largely an expansion of what NeilAway said, so feel free not to read it if you read his post. :-)

Jeff

[David Rajchenbach-Teller]

If this obsoletes some of mfbt/Scoped.h, can you remove/document this?

Thanks,
David

On 01/08/14 01:24, Jeff Walden wrote:
> Hey all,
>
> mfbt recently picked up mozilla::UniquePtr<T or T[]>, a smart pointer for objects created using |new| or |new[]|. UniquePtr will automatically |delete| or |delete[]| its pointer when it's destroyed. (You can also define custom deletion policies if you want.)
>

--
David Rajchenbach-Teller, PhD
Performance Team, Mozilla

[Jeff Walden]

On 08/05/2014 07:02 AM, David Rajchenbach-Teller wrote:
> If this obsoletes some of mfbt/Scoped.h, can you remove/document this?

I already replaced the file-description comment with

/* DEPRECATED: Use UniquePtr.h instead. */

and repeated the same thing at the very start of class-level documentation. Not sure what else there is to do on this front.

I suppose I could remove *all* the documentation and leave *only* deprecation notices, but it seems slightly better to keep it until the remaining users have been removed. (ScopedDeleteArray has already been removed, for starters.)

Jeff