Add a mozilla::Variant<T1, T2, ...> template class

RESOLVED FIXED in Firefox 42

Status

()

Core
MFBT
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: fitzgen, Assigned: fitzgen)

Tracking

(Blocks: 1 bug)

unspecified
mozilla42
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Something like boost::variant or Rust's enum, etc.

This is something I have wished we had in mfbt a few different times, so I finally just wrote it.
Created attachment 8622720 [details] [diff] [review]
Add a mozilla::Variant<T1, T2, ...> template class
Attachment #8622720 - Flags: review?(luke)
(In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #1)
> Created attachment 8622720 [details] [diff] [review]
> Add a mozilla::Variant<T1, T2, ...> template class

This implementation is pretty minimalistic and doesn't supply a method for matching on the variant type and ensuring that all possible variant cases are handled, but it would be super easy to add. Just wanted to start with the basics.
Created attachment 8622722 [details] [diff] [review]
Add a mozilla::Variant<T1, T2, ...> template class
Attachment #8622720 - Attachment is obsolete: true
Attachment #8622720 - Flags: review?(luke)
Attachment #8622722 - Flags: review?(luke)
Comment on attachment 8622722 [details] [diff] [review]
Add a mozilla::Variant<T1, T2, ...> template class

Apparently I was mistaken in my belief that Luke was an mfbt reviewer.

This version just removes the RTTI flags I was using for debugging template expansion and forgot to remove earlier.
Attachment #8622722 - Flags: review?(luke) → review?(jwalden+bmo)
Blocks: 1174916
Blocks: 1068988
Blocks: 1132288
Attachment #8622722 - Flags: review?(jwalden+bmo) → review?(nfroyd)
Blocks: 1178381

Comment 5

3 years ago
Comment on attachment 8622722 [details] [diff] [review]
Add a mozilla::Variant<T1, T2, ...> template class

I'm still looking at this, most of the way through.
Attachment #8622722 - Flags: review?(nfroyd) → review?(jwalden+bmo)
Comment on attachment 8622722 [details] [diff] [review]
Add a mozilla::Variant<T1, T2, ...> template class

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

Since Jeff has re-taken this review, I'll just post a few thoughts I had while skimming this.

::: mfbt/Variant.h
@@ +99,5 @@
> +class Variant;
> +
> +namespace detail {
> +
> +// Max computes the maximum sizeof(T) for each T in Ts.

Two thoughts here:

1) (non-optional) This would be better called MaxSizeof;
2) (optional) mfbt/TemplateLib.h has a Max template, so we could make that variadic and then replace this with:

template<typename... Ts>
struct MaxSizeof
{
  static const size_t size = tl::Max<sizeof(Ts)...>;
};

If you want to punt the second option to a followup, I think that's OK.

@@ +119,5 @@
> +// time, rather than at runtime. It ensures that the given type `Needle` is one
> +// of the types in the set of types `Haystack`.
> +
> +template<typename Needle, typename... Haystack>
> +struct IsVariant;

Wondering if this would be any better written something like:

template<typename Needle, typename... Haystack>
struct IsVariant
{
  static const bool value = tl::Or<IsSame<Needle, Haystack>::value...>::value;
};

You would, of course, have to write mozilla::tl::Or in TemplateLib.h, which should hopefully be easy.

We also ought to have checks somewhere, somehow, that the types in Haystack are distinct.  (i.e. Variant<int, int> and similar should be an error)

@@ +137,5 @@
> +template<typename Needle, typename T, typename... Haystack>
> +struct IsVariant<Needle, T, Haystack...> : public IsVariant<Needle, Haystack...> { };
> +
> +template<size_t N, typename T, typename U, typename Next, bool isMatch>
> +struct TagHelper;

This structure could stand a comment similar to the one you provided for IsVariant.
Attachment #8622722 - Flags: review?(jwalden+bmo) → review?(nfroyd)
Comment on attachment 8622722 [details] [diff] [review]
Add a mozilla::Variant<T1, T2, ...> template class

...and moving back the review because Bugzilla is lame.
Attachment #8622722 - Flags: review?(nfroyd) → review?(jwalden+bmo)

Comment 8

3 years ago
Comment on attachment 8622722 [details] [diff] [review]
Add a mozilla::Variant<T1, T2, ...> template class

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

::: mfbt/Variant.h
@@ +3,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +// # mozilla::Variant

I don't believe this will create a file-summary comment on MXR directory listings.  Add such a comment, consistent with the other files in mfbt.

Additionally, this is really a class-level overview comment.  Put it by the definition of Variant, and use a doxygen-style /** */ comment.

@@ +13,5 @@
> +// forbidden in C++ because it is not clear which type in the union should have
> +// its constructor and destructor run on creation and deletion
> +// respectively. This is the problem that `mozilla::Variant` solves.
> +//
> +// `mozilla::Variant` fully supports move-semantics.

Doesn't seem necessary to point this out -- these days, people will just expect this to work, in code we ourselves own, IMO.  A few stragglers might want to research the question rather than just try and see, but I expect they'd be the minority.

@@ +21,5 @@
> +// A `mozilla::Variant` instance is constructed (via move or copy) from one of
> +// its variant types.
> +//
> +//     Variant<char, uint32_t> v1('a');
> +//     Variant<UniquePtr<A>, B, C> v2(MakeUnique<A>());

And, you point out moving here already, so that's enough for even the reader looking for an answer.

It seems very much worth explicitly (heh) noting what constructors are supported.  Must the argument be *exactly* one of the parameter types?  Or are subclasses permitted to initialize?  Given the mess this file class probably is, I wouldn't want to have to figure this out by reading it, if I were the typical reader.  Please state explicitly that only the given types (ignoring const and references) are permitted.

@@ +102,5 @@
> +
> +// Max computes the maximum sizeof(T) for each T in Ts.
> +
> +template<typename T, typename... Ts>
> +struct Max

Name this MaxSizeof, or MaxSize, or something.  Max is way too general, especially for mozilla::detail.

@@ +161,5 @@
> +
> +template<size_t N, typename... Ts>
> +struct Implementation;
> +
> +// Empty Variant / recursion base case.

Empty Variant seems unuseful.  Why don't we make Variant<T> the base case?  Then we don't have this assert-false dead code, just a clear final consequence.  I'm perfectly fine relying on the EnableIf to ensure a proper type is passed.

@@ +251,5 @@
> +    return reinterpret_cast<void*>(&raw);
> +  }
> +
> +public:
> +  // Perfect forwarding construction for some variant type T.

All these interface comments should be doxygen-style /** */.

@@ +258,5 @@
> +           // perfect forwarding), so we have to remove those qualifiers here
> +           // when ensuring that T is a variant of this type, and getting T's
> +           // tag, etc.
> +           typename T = typename RemoveReference<typename RemoveConst<RefT>::Type>::Type,
> +           typename = typename EnableIf<detail::IsVariant<T, Ts...>::value, void>::Type>

It might be worth instead having

  static_assert(detail::IsVariant<T, Ts...>::value,
                "provided a type not found in this variant's type list");

which gives a nicer error message than SFINAE does.

@@ +260,5 @@
> +           // tag, etc.
> +           typename T = typename RemoveReference<typename RemoveConst<RefT>::Type>::Type,
> +           typename = typename EnableIf<detail::IsVariant<T, Ts...>::value, void>::Type>
> +  explicit Variant(RefT&& aT)
> +     : tag(Impl::template tag<T>())

Overindented a space.

@@ +302,5 @@
> +  }
> +
> +  // Check which variant type is currently contained.
> +  template<typename T,
> +           typename = typename EnableIf<detail::IsVariant<T, Ts...>::value, void>::Type>

Again, maybe worth a static_assert instead.

@@ +309,5 @@
> +  // Accessors for working with the contained variant value.
> +
> +  // Mutable reference.
> +  template<typename T,
> +           typename = typename EnableIf<detail::IsVariant<T, Ts...>::value, void>::Type>

static_assert

@@ +317,5 @@
> +  }
> +
> +  // Immutable const reference.
> +  template<typename T,
> +           typename = typename EnableIf<detail::IsVariant<T, Ts...>::value, void>::Type>

static_assert

@@ +326,5 @@
> +
> +  // Move the contained variant value out of this Variant<Ts...> container.
> +  template<typename T,
> +           typename = typename EnableIf<detail::IsVariant<T, Ts...>::value, void>::Type>
> +  T&& into() {

static_assert

This comment isn't really right -- it doesn't do any moving at all.  Whether moving happens, depends how the user uses the result of this method.  |v.into()| assigned into nothing, would do nothing.  |V v = v.into();| would do something.

If we want to actually guarantee an extraction, which the current method does not, we could have it return T, and T(Move(as())), to do so.  Then |v.into()| would always clear |v|.  What about if we did this, renaming the method to "extract"?

"""

Extract the contained variant value from this container into a temporary value.  On completion, the value in the variant will be in a safely-destructible state, as determined by the behavior of T's move constructor when provided the variant's internal value.
"""

@@ +328,5 @@
> +  template<typename T,
> +           typename = typename EnableIf<detail::IsVariant<T, Ts...>::value, void>::Type>
> +  T&& into() {
> +    MOZ_ASSERT(is<T>());
> +    return Move(*reinterpret_cast<T*>(&raw));

How about Move(as()) to kill off a cast?

::: mfbt/tests/TestVariant.cpp
@@ +19,5 @@
> +};
> +
> +int Destroyer::destroyedCount = 0;
> +
> +void

static on all these.

@@ +51,5 @@
> +{
> +  printf("testMove\n");
> +  Variant<UniquePtr<int>, char> v1(MakeUnique<int>(5));
> +  Variant<UniquePtr<int>, char> v2(Move(v1));
> +  MOZ_RELEASE_ASSERT(v2.is<UniquePtr<int>>());

Also

  MOZ_RELEASE_ASSERT(v1.is<UniquePtr<int>>());
  MOZ_RELEASE_ASSERT(v1.as<UniquePtr<int>>() == nullptr);

as v1 still contains a UniquePtr -- just a null one.

@@ +62,5 @@
> +
> +    Variant<char, UniquePtr<Destroyer>> v5('a');
> +    v5 = Move(v4);
> +
> +    auto ptr = v5.into<UniquePtr<Destroyer>>();

Add

  MOZ_RELEASE_ASSERT(Destroyer::destroyedCount == 0);

to verify the destructor's not called til the related variant is destroyed.

@@ +68,5 @@
> +  MOZ_RELEASE_ASSERT(Destroyer::destroyedCount == 1);
> +}
> +
> +void
> +testSizeOf()

Not sure why we need this test.  The size of a variant seems very much like it should be an implementation detail, no?
Attachment #8622722 - Flags: review?(jwalden+bmo) → review+
Created attachment 8627482 [details] [diff] [review]
Add a mozilla::Variant<T1, T2, ...> template class
Attachment #8622722 - Attachment is obsolete: true
Attachment #8627482 - Flags: review+
Blocks: 1178588
Blocks: 1178589
Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dbbb70357c69

(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #6)
> 1) (non-optional) This would be better called MaxSizeof;

Done.

> 2) (optional) mfbt/TemplateLib.h has a Max template, so we could make that
> variadic and then replace this with:

Unfortunately, mozilla::tl::Max only takes two arguments, not many. Will put it off as a follow up in bug 1178588.

> @@ +119,5 @@
> > +// time, rather than at runtime. It ensures that the given type `Needle` is one
> > +// of the types in the set of types `Haystack`.
> > +
> > +template<typename Needle, typename... Haystack>
> > +struct IsVariant;
> 
> Wondering if this would be any better written something like:
> 
> template<typename Needle, typename... Haystack>
> struct IsVariant
> {
>   static const bool value = tl::Or<IsSame<Needle,
> Haystack>::value...>::value;
> };
> 
> You would, of course, have to write mozilla::tl::Or in TemplateLib.h, which
> should hopefully be easy.

I'm not sure that `IsSame<Needle, Haystack>::value...` would expand as one would like it to, but maybe I am wrong?

> We also ought to have checks somewhere, somehow, that the types in Haystack
> are distinct.  (i.e. Variant<int, int> and similar should be an error)

Filed a follow up: bug 1178589.

> 
> @@ +137,5 @@
> > +template<typename Needle, typename T, typename... Haystack>
> > +struct IsVariant<Needle, T, Haystack...> : public IsVariant<Needle, Haystack...> { };
> > +
> > +template<size_t N, typename T, typename U, typename Next, bool isMatch>
> > +struct TagHelper;
> 
> This structure could stand a comment similar to the one you provided for
> IsVariant.

Done.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #8)
> @@ +258,5 @@
> > +           // perfect forwarding), so we have to remove those qualifiers here
> > +           // when ensuring that T is a variant of this type, and getting T's
> > +           // tag, etc.
> > +           typename T = typename RemoveReference<typename RemoveConst<RefT>::Type>::Type,
> > +           typename = typename EnableIf<detail::IsVariant<T, Ts...>::value, void>::Type>
> 
> It might be worth instead having
> 
>   static_assert(detail::IsVariant<T, Ts...>::value,
>                 "provided a type not found in this variant's type list");
> 
> which gives a nicer error message than SFINAE does.

This works for is/as/extract, however if we do this with the perfect forwarding ctor, then it captures template unification that would otherwise result in calling copy ctors and then we get the compile time error. So still using EnableIf for this perfect forwarding ctor, but the others use static_assert.

> If we want to actually guarantee an extraction, which the current method
> does not, we could have it return T, and T(Move(as())), to do so.  Then
> |v.into()| would always clear |v|.  What about if we did this, renaming the
> method to "extract"?
> 
> """
> 
> Extract the contained variant value from this container into a temporary
> value.  On completion, the value in the variant will be in a
> safely-destructible state, as determined by the behavior of T's move
> constructor when provided the variant's internal value.
> """

Done.
RyanVM tells me that the b2g failures in that try push are a known thing / not caused by my patch.
https://hg.mozilla.org/mozilla-central/rev/6509d3f6a91f
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Blocks: 1209227
You need to log in before you can comment on or make changes to this bug.