953296 - Implement mozilla::UniquePtr

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Jeff Walden [:Waldo]]

People want this.  Plus it's a fun to implement as a vacation project.  :-)

One semi-substantial pitfall to this is that it works simplest with new/delete-allocated things.  Which is almost nothing in the JS engine.  So the JS engine probably wants JS::UniquePtr automatically using js_free or js_delete.  But without C++11 template aliases you have to have a mozilla::Vector/VectorBase and JS::Vector scheme.  Bleh.  And more horribly bleh, because we don't have inherited constructors yet in our supported compilers.  But UniquePtr's constructors are arguably the most interesting part of the entire thing.  For vector was one thing; for here, that seems dangerous enough to not be something we want to do.

So, this is great if you use new/delete.  Otherwise, hmm.  At least in the short run, such cases will be a bit more typing, if they get used there.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Partial work (requires a few TypeTraits.h additions), simple use compiles

Comment 2

[Jeff Walden [:Waldo]]

Attachment: 1 - Add IsRvalueReference, IsReference

Patch series coming up.  I needed various separable bits of change to make it all work, so there are a few of those before UniquePtr itself.

This change adds reference-detection type traits, needed for deleter-as-reference behavior.

Comment 3

[Jeff Walden [:Waldo]]

Attachment: 2 - IsArray

This adds IsArray<T>::value, needed for the upcasting UniquePtr move constructor and move assignment operator (that is, |u = Move(u2);| where the owned thing and the deleter in |u2| have subclass-ish relationships to the same in |u|), to prevent assigning a UniquePtr<T[]> into a UniquePtr<T>.

Comment 4

[Jeff Walden [:Waldo]]

Attachment: 3 - IsEmpty

It appears from try results that every compiler on tinderbox, at least, implements __is_empty.  So add the trait for it.  This is needed to make UniquePtr instantiated with an empty deleter class not take up space inside such UniquePtrs (so UniquePtr is as size-efficient as a raw pointer, so people won't be scared off of using it).

Comment 5

[Jeff Walden [:Waldo]]

Attachment: 4 - Various NullPtr.h and related simplifications, additions, changes

Implementing operator=(nullptr_t) and UniquePtr(nullptr_t) and such requires a lot of care, because we don't actually have true nullptr all the time (specifically, with gcc 4.4 and 4.5).  And decltype(nullptr) isn't good enough, because decltype(__null) is int or long, but you can't perfectly forward a null pointer constant *unless* it's nullptr.  So, hacks.

This patch adds mozilla::IsNullPointer from C++1y, which in concert with SFINAE is the preferred way to add overloads targeted at nullptr.  This assumes that __null and nullptr behavior should be to select an otherwise-identical sibling overload that accepts a pointer type.  That generally should be the case, or at least we can restrict ourselves to that for now.  (It seems better not to make NullPtr.h depend on TypeTraits.h, so no using IntegralConstant or TrueType/FalseType to implement this.)

But you can't use SFINAE when the method being declared/defined is an operator, because operators have to take exactly one or two arguments.  So for that we add mozilla::NullptrT, a type that nullptr (whether true or emulated) will select as the best viable function.  We use decltype(nullptr) for this if we have true nullptr, of course.  And if we don't, we use void* and add asserts to semi-enforce that people only pass nullptr to them, and not some other pointer.  Ugly, but given most people have nullptr now, good enough, seems to me.

This patch also updates a few places in the tree doing one-off hackarounds for the above stuff, fixing them to use this new functionality.  already_AddRefed can't use explicit for the one constructor because if it did, |return __null;| triggers "error: conversion from ‘long int’ to non-scalar type ‘already_AddRefed<nsIFile>’ requested" and similar. 

I also added some comments to Move.h about nullptr not being perfectly forwardable (and about the other issue with perfect forwarding), and I trimmed some flab from NullPtr.h that's no longer relevant now that MSVC10 is the minimum requirement, and that we require clang with nullptr -- also making the emulation stuff clearer, I think.

Comment 6

[Jeff Walden [:Waldo]]

Attachment: 5 - UniquePtr for non-array types

This isn't quite an exact implementation of the interface, but it's pretty close.  If someone's missing some bit of functionality (support for UniquePtr<void, FreeDeleter> where FreeDeleter can delete a void* comes to mind), they can add it later.

The array specialization is next, in a separate patch.

Comment 7

[Jeff Walden [:Waldo]]

Attachment: 6 - UniquePtr<T[]>

It might the case that some set of the private deletions here isn't exactly exactly exactly right, but it's been good enough to break the things I manually tested to break.  (I don't think there's a way to test this bustage, but I could be wrong.)

Comment 8

[Jeff Walden [:Waldo]]

Attachment: 7 - Grab-bag of changes of ScopedDeletePtr to UniquePtr, and the array versions

Need to get a few users of this out there to verify it works, seed the field, get some testing of it even when --disable-tests (which sadly is still possible right now).

glandium for the mozglue bits.

jgilbert for the canvas/GL/gfx bits.

bbondy for the maintenanceservice bits.

bjacob for the Android widget bit.

mwu for the gonk bit.

Comment 9

[Jeff Walden [:Waldo]]

Attachment: 8 - Convert the source-hook stuff to UniquePtr

And this, which somehow got separated out of the previous patch, just for the one JS engine bit that uses the default new/delete.

Comment 10

[Jeff Walden [:Waldo]]

https://tbpl.mozilla.org/?tree=Try&rev=44e3e2e6d35d is a try run of basically the patches here.  I think the only new additions are some static_asserts about the sizes of various UniquePtr instantiations, but EBO should be pretty reliable in all the compilers we care about, so it's unlikely that would make any difference in the results.

As regards JS with its allocation mechanisms, I guess we'll have to add a JS::FreePolicy class or something, and people will just have to type a bit more.  Or something.  Probably shouldn't let perfect be the enemy of good for this.  But that's a discussion to have in a JS engine forum, not here.

Comment 11

[Benoit Jacob [:bjacob] (mostly away)]

Could you elaborate on what unique_ptr is, how UniquePtr compares to std::unique_ptr, and how it differs from ScopedDeletePtr ?

Comment 12

[Jeff Walden [:Waldo]]

Sure, see the big documentation comment in mfbt/UniquePtr.h in attachment 8355571 [details] [diff] [review], mostly.  ;-)

Although, that comment doesn't really assume knowledge of unique_ptr because it's so new, only of auto_ptr.  UniquePtr is basically a copy of std::unique_ptr in mfbt style.  ScopedDeletePtr isn't move-safe, so it's substantially identical to auto_ptr in terms of its deficiencies.

Comment 13

[(no longer active)]

Comment on attachment 8355570 [details] [diff] [review]
4 - Various NullPtr.h and related simplifications, additions, changes

Review of attachment 8355570 [details] [diff] [review]:
-----------------------------------------------------------------

::: mfbt/NullPtr.h
@@ +53,5 @@
> + *   // foo(T*) *exactly* and foo(decltype(nullptr)), nothing else
> + *   void foo(T*) { }
> + *   template<typename U>
> + *   void foo(U,
> + *            typename EnableIf<!IsNullPointer<N>::value, int>::Type dummy = 0)

I think you meant U here.

@@ +97,5 @@
> +  typedef decltype(nullptr) NullptrT;
> +  template<>
> +  struct IsNullPointer<decltype(nullptr)> { static const bool value = true; };
> +  }
> +#  undef MOZ_HAVE_CXX11_NULLPTR

You should not undef this here.  This macro is used outside of this header, for example: http://mxr.mozilla.org/mozilla-central/source/js/public/RootingAPI.h#510.  Actually I guess you can fix that code if you want.

Comment 14

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 8355574 [details] [diff] [review]
7 - Grab-bag of changes of ScopedDeletePtr to UniquePtr, and the array versions

Review of attachment 8355574 [details] [diff] [review]:
-----------------------------------------------------------------

r=me on the widget/android/GfxInfo.h bit. My understanding is that the only difference that this change makes, is enable safe move semantics, which are not currently used here anyway.

Comment 15

[Jeff Walden [:Waldo]]

Attachment: 4 - Various NullPtr.h and related simplifications, additions, changes (updated)

Ugh, thanks for noticing the MOZ_HAVE_CXX11_NULLPTR instance I forgot to fix -- I do want to remove the macro entirely, to be clear.  I did a search for it a bit ago, but my eyes must have glazed over the first hit for the string.

Comment 16

[Joshua Cranmer [:jcranmer]]

Comment on attachment 8355563 [details] [diff] [review]
3 - IsEmpty

Review of attachment 8355563 [details] [diff] [review]:
-----------------------------------------------------------------

As a postlude, be careful when saying that a type trait extension is present on all compilers--MSVC's returns different values in a few cases (__is_pod) than gcc/clang. It doesn't appear to affect __is_empty or __is_class though.

::: mfbt/TypeTraits.h
@@ +214,5 @@
>  {};
>  
> +namespace detail {
> +
> +// __is_class is a supported extension across all of our supported compilers.

It may be worth adding links to the type traits extensions for the compilers somewhere in the file:
http://clang.llvm.org/docs/LanguageExtensions.html
http://msdn.microsoft.com/en-us/library/ms177194%28v=VS.100%29.aspx
http://gcc.gnu.org/onlinedocs/gcc/Type-Traits.html

[Maybe it's better to link to gcc-4.4's type traits docs page?]

@@ +340,5 @@
> +
> +} // namespace detail
> +
> +/**
> + *

EMISSINGDOCS?

::: mfbt/tests/TestTypeTraits.cpp
@@ +70,5 @@
>  static_assert(IsReference<int&>::value, "int& is a reference");
>  static_assert(IsReference<int&&>::value, "int&& is a reference");
>  
> +static_assert(IsEmpty<const volatile S1>::value, "S should be empty");
> +static_assert(!IsEmpty<U>::value, "S should be empty");

Don't you mean U should not be empty?

@@ +84,5 @@
> +struct S4 : S2 {};
> +
> +static_assert(IsEmpty<S3>::value, "S3 should be empty");
> +static_assert(IsEmpty<S4>::value, "S4 should be empty");
> +

Add an assert that IsEmpty<int>::value should be false?

Comment 17

[Jeff Walden [:Waldo]]

Parts 3 and 4 landed with tweaks and slight futzing to land them out of original order:

https://hg.mozilla.org/integration/mozilla-inbound/rev/e40099b1ffa2
https://hg.mozilla.org/integration/mozilla-inbound/rev/25252ec3c38e

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e40099b1ffa2
https://hg.mozilla.org/mozilla-central/rev/25252ec3c38e

Comment 19

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8355574 [details] [diff] [review]
7 - Grab-bag of changes of ScopedDeletePtr to UniquePtr, and the array versions

Review of attachment 8355574 [details] [diff] [review]:
-----------------------------------------------------------------

/.*GLContext.*/ looks good to me.

Comment 20

[Jeff Walden [:Waldo]]

Is{Rvalue,}Reference and IsArray landed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/3d519786b858
https://hg.mozilla.org/integration/mozilla-inbound/rev/a6330cc7cc46

Comment 21

[Mike Hommey [:glandium]]

Comment on attachment 8355574 [details] [diff] [review]
7 - Grab-bag of changes of ScopedDeletePtr to UniquePtr, and the array versions

Review of attachment 8355574 [details] [diff] [review]:
-----------------------------------------------------------------

What about the remaining ones under mozglue/linker/? Also, please land the mozglue/linker/ changes separately from the others, because it makes it easier to cherry-pick the changes to keep the github project up-to-date.

Comment 22

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/3d519786b858
https://hg.mozilla.org/mozilla-central/rev/a6330cc7cc46

Comment 23

[Joshua Cranmer [:jcranmer]]

Comment on attachment 8355571 [details] [diff] [review]
5 - UniquePtr for non-array types

Review of attachment 8355571 [details] [diff] [review]:
-----------------------------------------------------------------

For common reference, I'm going by N3797 [C++1y draft] for adherence to standard.

I notice you're also missing std::make_unique equivalents. Granted, we don't have variadic templates (grr, MSVC), but providing at least a no-args and maybe a one-arg or so constructor would be useful.

Also, it would be helpful for tests where the deleter is an lvalue-to-function type.

::: mfbt/UniquePtr.h
@@ +22,5 @@
> +template<typename T>
> +class DefaultDelete
> +{
> +  public:
> +    DefaultDelete() {}

Add a MOZ_CONSTEXPR here?

@@ +27,5 @@
> +
> +    template<typename U>
> +    DefaultDelete(const DefaultDelete<U>& other,
> +                  typename EnableIf<mozilla::IsConvertible<U*, T*>::value,
> +                                    int>::Type dummy = 0)

Stupid MSVC, not allowing function template delete arguments until 2013. :(

@@ +41,5 @@
> +template<typename T>
> +class DefaultDelete<T[]>
> +{
> +  public:
> +    DefaultDelete() {}

Ditto for a MOZ_CONSTEXPR here.

@@ +63,5 @@
> +// used as a base class.
> +template<typename Pointer, typename DeleterType,
> +         DeleterStorage =
> +           IsEmpty<DeleterType>::value ? DeleterAsBase : DeleterAsMember>
> +struct ContentsTuple;

This EBO-templated pair may be useful to people outside of just UniquePtr (I can see this being useful for implementing C++11-like hashtables, e.g.), so I would consider putting it in its own header file. Or maybe just doing some renaming such that it doesn't imply being limited to UniquePtr in terms of usefulness.

@@ +93,5 @@
> +    ContentsTuple(const ContentsTuple&) MOZ_DELETE;
> +};
> +
> +template<typename Pointer, typename DeleterType>
> +struct ContentsTuple<Pointer, DeleterType, DeleterAsBase> : DeleterType

Shouldn't this be : private DeleterType?

@@ +248,5 @@
> +                  "UniquePtr restrictions for array types (20.7.1.3) not "
> +                  "implemented yet (20.7.1.3)");
> +
> +  public:
> +    typedef T* Pointer;

Clause 3: If the type remove_reference<D>::type::pointer exists, then unique_ptr<T, D>::pointer shall be a synonym for that. Otherwise, it should be a synonym for T*.

[Of course, this also plays havoc with our naming schemes, should we ever wish to replace mozilla::UniquePtr with std::unique_ptr].

@@ +259,5 @@
> +  public:
> +    /**
> +     * Construct a UniquePtr containing |nullptr|.
> +     */
> +    UniquePtr()

Sprinkle some MOZ_CONSTEXPR goodness?

@@ +270,5 @@
> +    /**
> +     * Construct a UniquePtr containing |p|.
> +     */
> +    explicit UniquePtr(Pointer p)
> +      : tuple(p, DeleterType())

Strictly speaking, this is in violation of the spec, since you're not constructing in place but constructing and then moving it in place. However, I don't think we have much use of allocator stuff in our tree right now, so I don't know how big a deal this is.

@@ +285,5 @@
> +    {}
> +
> +    UniquePtr(Pointer p,
> +              typename RemoveReference<D>::Type&& d2)
> +      : tuple(Move(p), Move(d2))

The Move(p) should be just a regular p here.

@@ +315,5 @@
> +                                 : IsConvertible<E, D>::value),
> +                                int>::Type dummy = 0)
> +      : tuple(other.release(), Forward<E>(other.getDeleter()))
> +    {
> +    }

C++11 dictates a constructor for std::auto_ptr. Obviously we don't exactly have an equivalent for std::auto_ptr, but we do have four or five auto_ptr-like classes. It might be worth creating a constructor for easier transition that accepts any type for which T::forget() is a pointer that is convertible to U* for transition purposes.

@@ +364,5 @@
> +
> +  public:
> +    operator ConvertibleToBool() const {
> +      return get() != nullptr ? &UniquePtr::nonNull : nullptr;
> +    }

Sigh, too bad we don't have explicit operator bool.

@@ +439,5 @@
> +operator!=(NullptrT n, const UniquePtr<T, D>& x)
> +{
> +  MOZ_ASSERT(n == nullptr);
> +  return bool(x);
> +}

Any particular reason you're omitting >, <, >=, and <=?

::: mfbt/tests/TestUniquePtr.cpp
@@ +28,5 @@
> +  } while (false)
> +
> +typedef UniquePtr<int> NewInt;
> +static_assert(sizeof(NewInt) == sizeof(int*),
> +              "stored most efficiently");

Yay empty base class optimization! :-)

Comment 24

[Jeff Walden [:Waldo]]

I've been going straight from C++11.  But I haven't always picked up every little nook and cranny in the spec, in the interests of covering the 90% cases and filling out the flourishes as needed.  That explains no Deleter::Pointer, no operator(not equalities), maybe others.  (No auto_ptr was deliberate, as I didn't want to even slightly encourage use of auto_ptr.)  This is also why no MakeUnique, which does seem a reasonable thing to have.  (But see below.)

ContentsTuple was non-generic out of convenience.  I'd have skipped it entirely except for wanting UniquePtr<T> to be sizeof(T).  Also note ContentsTuple doesn't work with a MOZ_FINAL deleter.  These limitations probably would have to be worked around for a generalized thing.  Seems short-run best to just KISS here -- when initializer lists exist enough, maybe std::tuple will be usable.

Note that because we can't forward (emulated) nullptr, MakeUnique may often be crippled in practice.  :-\  I'm not sure how common it'll be for people to want to do that, but I wonder a lot about MakeUnique given that.

Comment 25

[Joshua Cranmer [:jcranmer]]

Comment on attachment 8355572 [details] [diff] [review]
6 - UniquePtr<T[]>

Review of attachment 8355572 [details] [diff] [review]:
-----------------------------------------------------------------

(feedback+ not so much because I found things wrong with it but because I want to play with it a bit before giving it r+).

::: mfbt/UniquePtr.h
@@ +386,4 @@
>  };
>  
>  template<typename T, class D>
> +class UniquePtr<T[], D>

A comment explaining how a UniquePtr<T> differs from a UniquePtr<T[]> would be useful.

@@ +400,5 @@
> +    /**
> +     * Construct a UniquePtr containing nullptr.
> +     */
> +    UniquePtr()
> +      : tuple(static_cast<Pointer>(nullptr), DeleterType())

This one's MOZ_CONSTEXPR as well.

@@ +434,5 @@
> +    {}
> +
> +    UniquePtr(Pointer p,
> +              typename RemoveReference<D>::Type&& d2)
> +      : tuple(Move(p), Move(d2))

As in the non-array case, this ought to be just tuple(p, Move(d2))

@@ +440,5 @@
> +      static_assert(!IsReference<D>::value,
> +                    "rvalue deleter can't be stored by reference");
> +    }
> +
> +  private:

You might want to have a comment here explaining why this constructor is deleted.

@@ +453,5 @@
> +    UniquePtr(UniquePtr&& other)
> +      : tuple(other.release(), Forward<DeleterType>(other.getDeleter()))
> +    {}
> +
> +    UniquePtr(decltype(nullptr))

Why are you using the decltype(nullptr) here instead of the more complex logic you used in the other patch?

@@ +526,5 @@
> +  private:
> +    UniquePtr(const UniquePtr& other) MOZ_DELETE; // construct using Move()!
> +    void operator=(const UniquePtr& other) MOZ_DELETE; // assign using Move()!
> +};
> +

The std::make_unique overload for T[] is missing, although this is an easy one (no variadic templates!).

Comment 26

[Jim Blandy :jimb]

Comment on attachment 8355575 [details] [diff] [review]
8 - Convert the source-hook stuff to UniquePtr

Review of attachment 8355575 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, and sorry for the wait!

Comment 27

[Frank Wein [:mcsmurf]]

Does this bug here basically resolve Bug 806546 (I think it's related)?

Comment 28

[Jeff Walden [:Waldo]]

It's pretty much impossible to fix that bug, so long as we have to worry about gcc 4.5.  I *think* (it's been awhile) comments in one of the patches here help elucidate when to use what typedef when you want to match nullptr and not other stuff.  That's kind of a poor second best.  But no, it doesn't add a typedef that matches nullptr, and only nullptr, because it can't.

Comment 29

[Jeff Walden [:Waldo]]

Deeper comments on the non-[] review bits.  Patches soonish, maybe hopefully later today.

(In reply to Joshua Cranmer [:jcranmer] from comment #23)
> I notice you're also missing std::make_unique equivalents. Granted, we don't
> have variadic templates (grr, MSVC), but providing at least a no-args and
> maybe a one-arg or so constructor would be useful.

Fair.  Might do in a separate patch or as followup, to get this there to start, tho.

> Also, it would be helpful for tests where the deleter is an
> lvalue-to-function type.

Fair.

> This EBO-templated pair may be useful to people outside of just UniquePtr (I
> can see this being useful for implementing C++11-like hashtables, e.g.), so
> I would consider putting it in its own header file. Or maybe just doing some
> renaming such that it doesn't imply being limited to UniquePtr in terms of
> usefulness.

Generalizing this wasn't as much trouble as I expected it might be.  Patch adding Pair.h coming up.

> Clause 3: If the type remove_reference<D>::type::pointer exists, then
> unique_ptr<T, D>::pointer shall be a synonym for that. Otherwise, it should
> be a synonym for T*.
> 
> [Of course, this also plays havoc with our naming schemes, should we ever
> wish to replace mozilla::UniquePtr with std::unique_ptr].

I think I have some work on this somewhere in my queue.  I'll push more on it for landing.

> @@ +270,5 @@
> > +    /**
> > +     * Construct a UniquePtr containing |p|.
> > +     */
> > +    explicit UniquePtr(Pointer p)
> > +      : tuple(p, DeleterType())
> 
> Strictly speaking, this is in violation of the spec, since you're not
> constructing in place but constructing and then moving it in place. However,
> I don't think we have much use of allocator stuff in our tree right now, so
> I don't know how big a deal this is.

I'm not so sure about this.  The unique_ptr in gcc 4.8 does things exactly as we do here, and the spec says to initialize the field with |p|, which I would take to mean doing what's done here.

> @@ +285,5 @@
> > +    {}
> > +
> > +    UniquePtr(Pointer p,
> > +              typename RemoveReference<D>::Type&& d2)
> > +      : tuple(Move(p), Move(d2))
> 
> The Move(p) should be just a regular p here.

Hmm.  That's again not what gcc 4.8's unique_ptr does.

> @@ +315,5 @@
> > +                                 : IsConvertible<E, D>::value),
> > +                                int>::Type dummy = 0)
> > +      : tuple(other.release(), Forward<E>(other.getDeleter()))
> > +    {
> > +    }
> 
> C++11 dictates a constructor for std::auto_ptr. Obviously we don't exactly
> have an equivalent for std::auto_ptr, but we do have four or five
> auto_ptr-like classes. It might be worth creating a constructor for easier
> transition that accepts any type for which T::forget() is a pointer that is
> convertible to U* for transition purposes.

Hmm.  I think probably we should stick to spec or so, and try to nudge people into switching somewhat more wholesale where possible.  (Plus less work for me now.  :-) )

> @@ +439,5 @@
> > +operator!=(NullptrT n, const UniquePtr<T, D>& x)
> > +{
> > +  MOZ_ASSERT(n == nullptr);
> > +  return bool(x);
> > +}
> 
> Any particular reason you're omitting >, <, >=, and <=?

Extra work possibly not worth it, mostly.  I think.  It's been awhile since I looked at this.

Comment 30

[Jeff Walden [:Waldo]]

Attachment: Introduce an EBO-performing Pair class to mfbt

No idea if the size assertions pass everywhere, I only tested clang.  Obviously will be try-servered before landing.

Comment 31

[Jeff Walden [:Waldo]]

(In reply to Joshua Cranmer [:jcranmer] (away June 7-12) from comment #25)
> A comment explaining how a UniquePtr<T> differs from a UniquePtr<T[]> would
> be useful.

The intent is for the single UniquePtr overview comment to be the sole reference for all things UniquePtr.  But I'll add a comment with a few-line explanation of the differences nonetheless.

> @@ +434,5 @@
> > +    {}
> > +
> > +    UniquePtr(Pointer p,
> > +              typename RemoveReference<D>::Type&& d2)
> > +      : tuple(Move(p), Move(d2))
> 
> As in the non-array case, this ought to be just tuple(p, Move(d2))

Again really not sure about this.

> You might want to have a comment here explaining why this constructor is
> deleted.

Fair.  Adding one by one, then referring to it in a comment by the other.

> @@ +453,5 @@
> > +    UniquePtr(UniquePtr&& other)
> > +      : tuple(other.release(), Forward<DeleterType>(other.getDeleter()))
> > +    {}
> > +
> > +    UniquePtr(decltype(nullptr))
> 
> Why are you using the decltype(nullptr) here instead of the more complex
> logic you used in the other patch?

Um, no idea.  Maybe idea-diversion between the two patches, since I've been applying many fixes to both parts separately, and maybe forgot this one.  Too long since I worked on this to say.  The more-complicated thing seems to work with 4.5/4.8 locally, switching to that.

Comment 32

[Jeff Walden [:Waldo]]

Attachment: Implement mozilla::RemoveExtent

Comment 33

[Jeff Walden [:Waldo]]

Attachment: Add IsLvalueReference and IsVoid

Comment 34

[Jeff Walden [:Waldo]]

Attachment: Implement mozilla::UniquePtr (T/T[] versions both)

I skipped Deleter::Pointer in the interests of dealing with the 95% case.  Given we don't have nullptr, it's hard to make this workable, at least for the first case I stumbled across (int file descriptor resource).

I added func& deleter tests.  MakeUnique is a separate patch in a sec.

Still not actually sure I believe "Strictly speaking, this is in violation of the spec".

Fixed both "should be just a regular p" instances, looks like a glibc++ bug.

Added "why T[] specialization" and "why are these overloads deleted" comments to the T[] version.

Used more complex nullptr overloading logic the one place, for consistency at absolute minimum.

I guess if Deleter::Pointer isn't happening in this bug, the IsVoid/AddLvalueReference patch isn't needed yet.  Oh well, might as well pocket this since it's done.

Comment 35

[Jeff Walden [:Waldo]]

Attachment: Implement mozilla::MakeUnique

I added [0, 4] argument forwarding versions.  If we want more, we can add more when it comes to it.  Lack of forwarding of nullptr is going to hurt this a lot in practice, I suspect.  :-\  Guess there's not much to do about that.  Maybe static_asserts if people run afoul of it on a regular basis or so.

Comment 36

[Jeff Walden [:Waldo]]

Attachment: Partial Deleter::Pointer support

This might be enough to make Deleter::Pointer work, but it needs tests, and I don't have a good enough handle on how to use it yet to write any.  And it's a rare case where it's necessary, more or less, anyway.  Punt!

Comment 37

[Joshua Cranmer [:jcranmer]]

Comment on attachment 8437825 [details] [diff] [review]
Introduce an EBO-performing Pair class to mfbt

Review of attachment 8437825 [details] [diff] [review]:
-----------------------------------------------------------------

The code in Pair.h looks good, but I'm concerned about the tests.

::: mfbt/tests/TestPair.cpp
@@ +31,5 @@
> +struct EmptyClass { EmptyClass(int) {} };
> +struct NonEmpty { char c; NonEmpty(int) {} };
> +
> +INSTANTIATE(int, EmptyClass, both1, sizeof(int));
> +INSTANTIATE(int, NonEmpty, both2, 2 * sizeof(int));

Add a copy of INSTANTIATE where EmptyClass is first and int is second.

@@ +41,5 @@
> +// class and first member of a struct.  (I'm not entirely certain how true this
> +// is with multiple base classes -- it may be implementation-defined depending
> +// on how the compiler lays out the classes.  So don't bother trying to test
> +// this with one class multiply inheriting from the other, until someone reports
> +// bustage.)

This comment is confusing and slightly inapplicable.

The basic rule of thumb with EBO is that empty subobjects of the same type must not point to the same address, which can be thought of as saying that Pair<A, A> x; &x.first() != x.second() must always hold true. For example:

struct Empty {};
struct AlsoEmpty : Empty {};
struct Foo : Empty, AlsoEmpty {};
struct Bar : Empty { AlsoEmpty e };

In this situation, Foo and Bar would both be size 2, not size 1. However, if AlsoEmpty didn't inherit from Empty, EBO would be applicable.

For your comment about multiple base classes, I think what you're trying to get at is that the layout order of base class subobjects is unspecified [which gives it leeway to be inconsistent!]. The leeway is I think mostly meant for virtual base classes, but I realized I was wrote the first draft of this sentence that the leeway could let compilers reorder base classes to make all empty base classes take 0-size.

@@ +44,5 @@
> +// this with one class multiply inheriting from the other, until someone reports
> +// bustage.)
> +INSTANTIATE(A, A, class1, 2);
> +INSTANTIATE(A, B, class2, 2);
> +INSTANTIATE(A, EmptyClass, class3, 1);

With that in mind, these tests don't really test anything, at least if you're trying to make reference to the above rule in the comment, as A is not an empty class where the potential for the EBO to break down exists.

Comment 38

[Jeff Walden [:Waldo]]

(In reply to Joshua Cranmer [:jcranmer] from comment #37)
> Add a copy of INSTANTIATE where EmptyClass is first and int is second.

INSTANTIATE itself tests both orderings.  (I had both orderings explicitly, then realized there was no good reason I couldn't do both orderings inside the macro.)

Comment 39

[Jeff Walden [:Waldo]]

Attachment: Pair, v2

Okay, tweaked the tests a bit, and made the comments slightly more precise.

Comment 40

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f2f60161a7ac
https://hg.mozilla.org/integration/mozilla-inbound/rev/d0ea5f3a97a7

Comment 41

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f2f60161a7ac
https://hg.mozilla.org/mozilla-central/rev/d0ea5f3a97a7

Comment 42

[Jeff Walden [:Waldo]]

Landed Pair, still waiting on UniquePtr itself:

https://hg.mozilla.org/integration/mozilla-inbound/rev/f0eb37be6b5f

Comment 43

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/f0eb37be6b5f

Comment 44

[Joshua Cranmer [:jcranmer]]

Comment on attachment 8438068 [details] [diff] [review]
Implement mozilla::UniquePtr (T/T[] versions both)

Review of attachment 8438068 [details] [diff] [review]:
-----------------------------------------------------------------

As I was reviewing this, I came to the realization that we may be subtly exposed to ODR bugs when it comes to operator delete [if you have UniquePtr<Foo> in a file that includes mozalloc.h transitively and another copy in a file that doesn't include it] Of course, on further reflection, nsAutoPtr should also be equally vulnerable, so the fact that we haven't hit it at all means it's not worth worrying about.

::: mfbt/UniquePtr.h
@@ +534,5 @@
> +  MOZ_ASSERT(n == nullptr);
> +  return bool(x);
> +}
> +
> +// No operator<, operator>, operator<=, operator>= for now because simplicity.

On reflection, I think the biggest reason to support these is when you're using an ordered data structure on UniquePtr... std::map comes to mind.

So not implementing these for the present is fine to me.

Comment 45

[Jeff Walden [:Waldo]]

Landed, with some of the earlier patches augmented trivially to use MakeUnique instead of .reset(new ...):

https://hg.mozilla.org/integration/mozilla-inbound/rev/93e619d0d2a4
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5045c3bc581
https://hg.mozilla.org/integration/mozilla-inbound/rev/0730e6e7639d
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b47204c8640
https://hg.mozilla.org/integration/mozilla-inbound/rev/beaf149165f9
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba853c3d6040
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ec39bb8c834
https://hg.mozilla.org/integration/mozilla-inbound/rev/b140663e3681

It turns out MSVC10 miscompiles mozilla::RemoveReference<function reference>, so function reference deleters are out for now.  This showed up as TestUniquePtr.cpp miscompiling.  I added an #if-guard to prevent the function deleter tests being compiled with MSVC compilers.  (Technically could have enabled >10, but didn't think it worth the effort since it's useless now anyway.)  Whenever we drop MSVC10, we can reenable that.  (Although, I tend to think function reference deletion is not really a feature that should be used -- much better for deleters to have known static behavior, for efficiency reasons and all.)

I filed bug 1035966 on adding Deleter::Pointer support, so this bug's done now.  \o/

Comment 46

[Jeff Walden [:Waldo]]

And https://hg.mozilla.org/integration/mozilla-inbound/rev/8bba49067049 because I forgot to also condition the TestFunctionDeleter call on the compiler being used.

Comment 47

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/93e619d0d2a4
https://hg.mozilla.org/mozilla-central/rev/a5045c3bc581
https://hg.mozilla.org/mozilla-central/rev/0730e6e7639d
https://hg.mozilla.org/mozilla-central/rev/0b47204c8640
https://hg.mozilla.org/mozilla-central/rev/beaf149165f9
https://hg.mozilla.org/mozilla-central/rev/ba853c3d6040
https://hg.mozilla.org/mozilla-central/rev/9ec39bb8c834
https://hg.mozilla.org/mozilla-central/rev/b140663e3681
https://hg.mozilla.org/mozilla-central/rev/8bba49067049

Comment 48

[Jeff Walden [:Waldo]]

Blog post:

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

And newsgroup announcement to accompany it:

https://groups.google.com/forum/#!topic/mozilla.dev.platform/Ji3T5kipJzA

(These in addition to all the comments in UniquePtr.h itself, of course.)