Closed Bug 953296 Opened 6 years ago Closed 6 years ago

Implement mozilla::UniquePtr

Categories

(Core :: MFBT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: Waldo, Assigned: Waldo)

References

Details

Attachments

(13 files, 4 obsolete files)

5.08 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
2.25 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
3.25 KB, patch
jcranmer
: review+
Details | Diff | Splinter Review
7.16 KB, patch
jcranmer
: feedback+
Details | Diff | Splinter Review
12.48 KB, patch
glandium
: review+
jgilbert
: review+
bbondy
: review+
bjacob
: review+
mwu
: review+
Details | Diff | Splinter Review
4.19 KB, patch
jimb
: review+
Details | Diff | Splinter Review
10.66 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
2.53 KB, patch
jcranmer
: review+
Details | Diff | Splinter Review
3.89 KB, patch
jcranmer
: review+
Details | Diff | Splinter Review
30.68 KB, patch
jcranmer
: review+
Details | Diff | Splinter Review
4.55 KB, patch
jcranmer
: review+
Details | Diff | Splinter Review
2.24 KB, patch
Details | Diff | Splinter Review
8.87 KB, patch
jcranmer
: review+
Details | Diff | Splinter Review
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.
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.
Attachment #8351597 - Attachment is obsolete: true
Attachment #8355559 - Flags: review?(nfroyd)
Attached patch 2 - IsArraySplinter Review
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>.
Attachment #8355560 - Flags: review?(nfroyd)
Attached patch 3 - IsEmptySplinter Review
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).
Attachment #8355563 - Flags: review?(Pidgeot18)
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.
Attachment #8355570 - Flags: review?(ehsan)
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.
Attachment #8355571 - Flags: review?(Pidgeot18)
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.)
Attachment #8355572 - Flags: review?(Pidgeot18)
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.
Attachment #8355574 - Flags: review?(netzen)
Attachment #8355574 - Flags: review?(mwu)
Attachment #8355574 - Flags: review?(mh+mozilla)
Attachment #8355574 - Flags: review?(jgilbert)
Attachment #8355574 - Flags: review?(bjacob)
And this, which somehow got separated out of the previous patch, just for the one JS engine bit that uses the default new/delete.
Attachment #8355575 - Flags: review?(jimb)
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.
Could you elaborate on what unique_ptr is, how UniquePtr compares to std::unique_ptr, and how it differs from ScopedDeletePtr ?
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 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.
Attachment #8355570 - Flags: review?(ehsan) → review-
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.
Attachment #8355574 - Flags: review?(bjacob) → review+
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.
Attachment #8355570 - Attachment is obsolete: true
Attachment #8355621 - Flags: review?(ehsan)
Attachment #8355621 - Flags: review?(ehsan) → review+
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?
Attachment #8355563 - Flags: review?(Pidgeot18) → review+
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
Whiteboard: [leave open]
Attachment #8355574 - Flags: review?(mwu) → review+
Attachment #8355559 - Flags: review?(nfroyd) → review+
Attachment #8355560 - Flags: review?(nfroyd) → review+
Attachment #8355574 - Flags: review?(netzen) → review+
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.
Attachment #8355574 - Flags: review?(jgilbert) → review+
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.
Attachment #8355574 - Flags: review?(mh+mozilla) → review+
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! :-)
Attachment #8355571 - Flags: review?(Pidgeot18) → feedback+
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 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!).
Attachment #8355572 - Flags: review?(Pidgeot18) → feedback+
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!
Attachment #8355575 - Flags: review?(jimb) → review+
Does this bug here basically resolve Bug 806546 (I think it's related)?
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.
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.
No idea if the size assertions pass everywhere, I only tested clang.  Obviously will be try-servered before landing.
Attachment #8437825 - Flags: review?(Pidgeot18)
(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.
Attachment #8437968 - Flags: review?(Pidgeot18)
Attachment #8437969 - Flags: review?(Pidgeot18)
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.
Attachment #8438068 - Flags: review?(Pidgeot18)
Attachment #8355571 - Attachment is obsolete: true
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.
Attachment #8438069 - Flags: review?(Pidgeot18)
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 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.
Attachment #8437825 - Flags: review?(Pidgeot18) → review-
Attachment #8437968 - Flags: review?(Pidgeot18) → review+
Attachment #8437969 - Flags: review?(Pidgeot18) → review+
(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.)
Attached patch Pair, v2Splinter Review
Okay, tweaked the tests a bit, and made the comments slightly more precise.
Attachment #8441006 - Flags: review?(Pidgeot18)
Attachment #8437825 - Attachment is obsolete: true
Attachment #8441006 - Flags: review?(Pidgeot18) → review+
Attachment #8438069 - Flags: review?(Pidgeot18) → review+
Blocks: 1030922
Landed Pair, still waiting on UniquePtr itself:

https://hg.mozilla.org/integration/mozilla-inbound/rev/f0eb37be6b5f
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.
Attachment #8438068 - Flags: review?(Pidgeot18) → review+
Blocks: 1035966
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/
Keywords: leave-open
Target Milestone: --- → mozilla33
And https://hg.mozilla.org/integration/mozilla-inbound/rev/8bba49067049 because I forgot to also condition the TestFunctionDeleter call on the compiler being used.
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.)
Depends on: 1055044
Depends on: 1120062
You need to log in before you can comment on or make changes to this bug.