Closed Bug 1338389 Opened 7 years ago Closed 7 years ago

Allow same types in Variant (or create a separate class)

Categories

(Core :: MFBT, defect)

Product:
Core
Component:
MFBT
Version:
53 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: mozbugz, Assigned: mozbugz)

References

(Blocks 2 open bugs)

Details

Attachments

(4 files)

Bug 1338389 - Index-based Variant::is<N>, as<N>, and extract<N> -
59 bytes, text/x-review-board-request
froydnj
: review+
Details
Bug 1338389 - Allow repeated Variant types, but prevent is/as/extract<T> for them -
59 bytes, text/x-review-board-request
froydnj
: review+
Details
Bug 1338389 - VariantType<T> and VariantIndex<N> permit unambiguous and variadic Variant construction -
59 bytes, text/x-review-board-request
froydnj
: review+
Details
Bug 1338389 - Tests for Variant's detail::Nth and detail::SelectVariantType -
59 bytes, text/x-review-board-request
froydnj
: review+
Details 
Currently MFBT's Variant forces all types to be different, which makes sense because all accesses use types to distinguish which item is targeted.
E.g., for:
> Variant<int, int> vii(42);
> vii.as<int>();
which of the two possible ints should we consider?

However in some situations it would be great to allow same types, and access them by an index in the type list. Borrowing some ideas from std::variant:
> Variant<int, int> vii(InPlaceIndex<1>, 42); // Assign to the 2nd int
> assert(vii.index() == 1);
> assert(vii.is<1>());
> assert(vii.as<1>() == 42);
> vii.as<1>() += 1;
> vii = AsVariant<0>(123);
Etc.

One use-case would be MozPromise::ResolveOrRejectValue<ResolveValue, RejectValue> (where the two types could be the same), which currently uses two Maybe's, though only one of them can contain something at any time, so we waste some space to store both separately!
Instead, it could use `Variant<Nothing, ResolveType, RejectType>`, and we'd always refer to them by index.


If we don't want to pollute Variant with same types and indices, we could just create a separate class; `IndexedVariant`?
It could even be used to re-implement Variant, to keep future maintenance costs low.
WIP: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c835b6d8ec9bd05b2b50b77878c23b2645fc82d1

Featuring:
- Removed `bool` tag, I think it's useless.
- Bigger tag type is `uint_fast32_t` instead of `size_t` -- Who would give more than 2^32 template arguments? :-P
- `emplace`
- `VariantType<T>` and `VariantIndex<N>` can be used as first arg of constructor or `emplace`, to unambiguously select the target type (useful for conversions, and zero- or multi-arguments construction; and can more efficiently replace `AsVariant` is some cases).
- Index-based `is<N>()`, `as<N>()`, and `extract<N>()`. Also `index()` to dynamically get current index number.
- Allow types to appear more than once, but these may only be accessed through indices.


Nathan:
If you're reading this, I'm thinking of making you review this!
So feel free to give it a glance and early feedback, or let me know if I should ask someone else...
Assignee: nobody → gsquelart
(In reply to Gerald Squelart [:gerald] from comment #1)
> - Removed `bool` tag, I think it's useless.
> - Bigger tag type is `uint_fast32_t` instead of `size_t` -- Who would give
> more than 2^32 template arguments? :-P

Aside from nitpickiness, what's wrong with the existing code?  Add a uint32_fast_t variant if you must, but it seems unnecessary to muck with this, and you're undoing deliberate changes.

> - `emplace`

I've noticed this lack before.  I think the lack is, or should be, deliberate.  If you have a Variant containing a value of a type, you know it contains that value and type forever.  A match on a Variant remains valid forever; nothing can invalidate it.

But once you add emplace, the type and value in a Variant can change.  Matches are no longer permanently correct.  Most of all, if you pass around mutable references or pointers, the presumption must be that guarantees go out the window.

The ability to emplace isn't a bad thing.  But it's probably best relegated to a separate type.  And indeed, we have more or less have such a type: MaybeOneOf.  All that needs to happen to that type, is for it to be comprehensively variadic, rather than totally special-casing two types.  (And conceivably rename it, and maybe make empty an optional state.)  Would be nice if the storage mechanism beneath were shared, even, but that's getting to be a bigger and bigger change.
(In reply to Jeff Walden [:Waldo] (not taking new requests -- poke me on IRC if it's important) from comment #2)

Thank you for the feedback, always appreciated in this kind of code.


> (In reply to Gerald Squelart [:gerald] from comment #1)
> > - Removed `bool` tag, I think it's useless.
> > - Bigger tag type is `uint_fast32_t` instead of `size_t` -- Who would give
> > more than 2^32 template arguments? :-P
> 
> Aside from nitpickiness, what's wrong with the existing code?  Add a
> uint32_fast_t variant if you must, but it seems unnecessary to muck with
> this, and you're undoing deliberate changes.

I see you've done that in bug 1287243.

I'm not really undoing your changes, there is still a type optimization done on the tag, depending on the number of Variant alternatives. I'm just tweaking the chosen types...

My thinking was:
* for `bool`:
- If uint_fast8_t is 1 byte, then `bool` doesn't bring any size benefit.
- If uint_fast8_t is bigger than 1 byte, then `bool` can indeed be smaller, but it will also presumably be slower.
- Using C++ `bool` to record something else than `true` or `false` feels weird to me these days. :-)
Based on all this, I thought the `bool` option was not worth worrying about. But I admit I haven't studied this change in depth (generated code, performance...)

* for `size_t`:
- In practice we should never encounter 2^32-argument Variant, so it seemed wasteful to always use size_t which can be 64 bits, when a fast 32-bit value should be enough, and gain a bit of space.

If bool/fast8/size_t is in fact the best choice ever, never to be changed, I think it'd be nice to put a comment in the code to explain that, and avoid another enthusiastic developer from trying to change it again in the future. ;-)

Anyway, it was just a fly-by change for me while I was there, I'm happy to drop it from this bug -- in which case, any objection against me opening a separate bug to revisit it on its own?


> > - `emplace`
> 
> I've noticed this lack before.  I think the lack is, or should be,
> deliberate.  If you have a Variant containing a value of a type, you know it
> contains that value and type forever.  A match on a Variant remains valid
> forever; nothing can invalidate it.
> 
> But once you add emplace, the type and value in a Variant can change. [...]

I'm not sure I follow... You can already change a Variant's type and value by assigning from another Variant object, or from an AsVariant expression.
Oh, about `MaybeOneOf`: It could be replaced with `Variant<Nothing, T1, T2>`! ;-)

(Except for the part where Variant requires an initial value, so we'd have to explicitly construct it with a `Nothing`... But then we could just make Variant default-initialize itself with the first type, like std::variant.)
Comment on attachment 8865367 [details]
Bug 1338389 - Allow repeated Variant types, but prevent is/as/extract<T> for them -

https://reviewboard.mozilla.org/r/137026/#review140126

I'm still a little skeptical, but the example of using `Variant` in templated types provides ideas on how this could be useful.  Comments below.

::: mfbt/Variant.h:44
(Diff revision 1)
> -template <typename...>
> -struct FirstTypeIsInRest;
> -
> -template <typename First>
> +// IsNotVariant<T, Ts...> will be TrueType if T in not in Ts.
> +// This is used by IsVariant to ensure that a type only appears once.
> +template<typename Needle, typename... Haystack>
> +struct IsNotVariant;

This struct needs a different name.

::: mfbt/Variant.h:58
(Diff revision 1)
>  // The `IsVariant` helper is used in conjunction with static_assert and
>  // `mozilla::EnableIf` to catch passing non-variant types to `Variant::is<T>()`
>  // and friends at compile time, rather than at runtime. It ensures that the
> -// given type `Needle` is one of the types in the set of types `Haystack`.
> -
> +// given type `Needle` is one of the non-repeating types in the set of types
> +// `Haystack`.
>  template<typename Needle, typename... Haystack>
>  struct IsVariant;

This one wasn't great before, but it definitely needs a new name now.

::: mfbt/Variant.h:354
(Diff revision 1)
> - * Either the stored type, or the type index may be provided.
> + * Either the stored type, or the type index may be provided. If a type is
> + * present more than once, each of its instances can only be accessed by index.
>   *
>   *     void
>   *     Foo(Variant<A, B, C, B> v)
>   *     {
>   *       if (v.is<A>()) {
>   *         A& ref = v.as<A>();
>   *         ...
> - *       } else (v.is<1>()) { // Instead of v.is<B>, which is ambiguous.
> + *       } else (v.is<1>()) { // Instead of v.is<B>, which is not allowed.

I think an example with duplicate types should be entirely separate, along with rationale (e.g. duplicate types would usually only come up when using Variant in templated types).  It's not really obvious why duplicate types are useful in this example, and the duplicated-ness isn't really explained well, even with the modifications you made.

::: mfbt/tests/TestVariant.cpp:45
(Diff revision 1)
> +testDuplicate()
> +{
> +  printf("testDuplicate\n");
> +  Variant<uint32_t, uint64_t, uint32_t> v(uint64_t(1));
> +  MOZ_RELEASE_ASSERT(v.is<uint64_t>());
> +  //MOZ_RELEASE_ASSERT(!v.is<uint32_t>()); // uint32_t is not unique!

Just remove this, then?
Attachment #8865367 - Flags: review?(nfroyd) → review-
Comment on attachment 8865366 [details]
Bug 1338389 - Index-based Variant::is<N>, as<N>, and extract<N> -

https://reviewboard.mozilla.org/r/137024/#review140138

::: mfbt/Variant.h:368
(Diff revision 1)
> + * Either the stored type, or the type index may be provided.
>   *
>   *     void
> - *     Foo(Variant<A, B, C> v)
> + *     Foo(Variant<A, B, C, B> v)
>   *     {
>   *       if (v.is<A>()) {
>   *         A& ref = v.as<A>();
>   *         ...
> + *       } else (v.is<1>()) { // Instead of v.is<B>, which is ambiguous.
> + *         ...

I'm ambivalent which part cleans this comment up, as mentioned elsewhere, but it definitely needs to be done.

::: mfbt/Variant.h:569
(Diff revision 1)
> +  /** Index of the currently-stored type, starting at 0. */
> +  size_t index() const
> +  {
> +    return size_t(tag);
> +  }

I am inclined to say that we don't need this, as we don't want to expose what essentially amounts to the tag value, and generally the interesting uses of an index of a type all come from a compile-time constant anyway.  I see that we do use it for asserts, so perhaps we should make it private?
Attachment #8865366 - Flags: review?(nfroyd) → review-
Comment on attachment 8865368 [details]
Bug 1338389 - VariantType<T> and VariantIndex<N> permit unambiguous and variadic Variant construction -

https://reviewboard.mozilla.org/r/137028/#review140234

::: mfbt/Variant.h:352
(Diff revision 1)
> - * AsVariant() must copy or move the value into a temporary and this cannot
> - * necessarily be elided by the compiler, it's mostly appropriate only for use
> + * A. Brace-construction with VariantType or VariantIndex; this also allows
> + * in-place construction with zero, or with multiple arguments.

Why is this described as "with zero, or with multiple arguments"...what happened to the "one argument" case?

::: mfbt/Variant.h:358
(Diff revision 1)
> - * with primitive or very small types.
>   *
> + *     struct AB { AB(int, int){...} };
> + *     static Variant<AB, bool> foo()
> + *     {
> + *       return {VariantIndex<0>{}, 1, 2};

It is funny to see this described as "easier" compared to `AsVariant`. :)

::: mfbt/Variant.h:521
(Diff revision 1)
>  
>    /**
> +   * Perfect forwarding construction for some variant type T, by
> +   * explicitly giving the type.
> +   * This is necessary to construct from zero or multiple arguments,
> +   * or to convert from a foreign type.

What is "foreign" type here?  A type that requires conversion to one of the types in the variant?

Reading this initially made me think of FFI, which I don't think is in play here.  I think we need to find a different way of talking about this sort of type.

::: mfbt/Variant.h:534
(Diff revision 1)
> +
> +  /**
> +   * Perfect forwarding construction for some variant type T, by
> +   * explicitly giving the type index.
> +   * This is necessary to construct from zero or multiple arguments,
> +   * to convert from a foreign type,

Likewise here.

::: mfbt/Variant.h:539
(Diff revision 1)
> +   * to convert from a foreign type,
> +   * or to construct a type that is present more than once in the Variant.
> +   */
> +  template<size_t N,
> +           typename... Args,
> +           typename T = typename detail::Nth<N, Ts...>::Type>

Can we not put this in the template args, but put it as a local typedef or something?  Putting temporary types in template arguments unnecessarily, especially in public interfaces, bugs me.

::: mfbt/tests/TestVariant.cpp:69
(Diff revision 1)
> +  //MOZ_RELEASE_ASSERT(v.is<uint32_t>()); // uint32_t is not unique!
> +  //MOZ_RELEASE_ASSERT(v.as<uint32_t>() == 1);

Just...not have these tests?  Or just comment `// no tests for as<uint32_t> because it's ambiguous which uint32_t we're referring to`?
Attachment #8865368 - Flags: review?(nfroyd)
Comment on attachment 8865366 [details]
Bug 1338389 - Index-based Variant::is<N>, as<N>, and extract<N> -

https://reviewboard.mozilla.org/r/137024/#review140138

Thank you for the thorough review.

> I'm ambivalent which part cleans this comment up, as mentioned elsewhere, but it definitely needs to be done.

In fact, this example is incorrect in this first patch, as repeated types are not allowed yet.
For `v.is<1>()`, I'll just change the comment to say "same as v.is<B>() in this case." I'll elaborate on repeated types separately in the next patch.

> I am inclined to say that we don't need this, as we don't want to expose what essentially amounts to the tag value, and generally the interesting uses of an index of a type all come from a compile-time constant anyway.  I see that we do use it for asserts, so perhaps we should make it private?

For this one, I lifted the idea from C++17's std::variant.
But for most public uses, `is<N>()` should be enough. So I'll remove `index()`.
Comment on attachment 8865367 [details]
Bug 1338389 - Allow repeated Variant types, but prevent is/as/extract<T> for them -

https://reviewboard.mozilla.org/r/137026/#review140126

> This struct needs a different name.

...

> This one wasn't great before, but it definitely needs a new name now.

This made me re-read the code and realize that IsVariant and IsNotVariant (and even TypesAreDistinct before) actually don't correctly handle const and reference types:
If you look at SelectVariantType, it first strips any reference and const from the provided type, and then compare that to reference- and const-stripped types from the Variant! E.g.: `Foo&` would select the first type in `Variant<const Foo, ...>`.
This also means that selecting `Foo` in `Variant<Foo, Foo&, const Foo, ...> is ambiguous (though the original code just picked the first one, and TypesAreDistinct wouldn't have caught the ruse.)

So now I will implement a `CountSelectableVariantTypes<T, Variants...>` which will count the number of Variant types that could be selected for a given T. And I will sprinkle `static_assert`s to make sure we only try to unambiguously select exactly one type from the Variant.

> I think an example with duplicate types should be entirely separate, along with rationale (e.g. duplicate types would usually only come up when using Variant in templated types).  It's not really obvious why duplicate types are useful in this example, and the duplicated-ness isn't really explained well, even with the modifications you made.

Alright I'll make a separate example with hopefully a better rationale (something like a templated class using Variant internally.)

> Just remove this, then?

I was trying to show which lines from the Simple test were not allowed in the Duplicate case. But happy to change it into a note.
Comment on attachment 8865368 [details]
Bug 1338389 - VariantType<T> and VariantIndex<N> permit unambiguous and variadic Variant construction -

https://reviewboard.mozilla.org/r/137028/#review140234

> Why is this described as "with zero, or with multiple arguments"...what happened to the "one argument" case?

The "also" word is key! I was just showing that compared to the constructor with just one T argument, using VariantType/VariantIndex *also* allows for 0 or >=2 args. But yeah it's a bit awkward, so I'll change it to "this allows in-place construction with any number of arguments".

> It is funny to see this described as "easier" compared to `AsVariant`. :)

I meant that both brace-init and AsVariant are easier than specifying a full Variant<AllTheTs...> constructor.

> What is "foreign" type here?  A type that requires conversion to one of the types in the variant?
> 
> Reading this initially made me think of FFI, which I don't think is in play here.  I think we need to find a different way of talking about this sort of type.

Ok, I'll remove the ambiguous "foreign" and just say "convert from a type that is not in the Variant's type list."

> Can we not put this in the template args, but put it as a local typedef or something?  Putting temporary types in template arguments unnecessarily, especially in public interfaces, bugs me.

I was starting to like it! :-)
But I see that it can be confusing to add this in the template parameter list, and it can just live inside the function.
Also, it could SFNINAE-hide the function if N is too big, resulting in a less obvious error.
Comment on attachment 8865366 [details]
Bug 1338389 - Index-based Variant::is<N>, as<N>, and extract<N> -

https://reviewboard.mozilla.org/r/137024/#review142576

r=me with some fodder for followups and/or further discussion here.

::: mfbt/Variant.h:253
(Diff revision 2)
>    template<typename Variant>
>    static void moveConstruct(void* aLhs, Variant&& aRhs) {
> -    if (aRhs.template is<T>()) {
> -      ::new (KnownNotNull, aLhs) T(aRhs.template extract<T>());
> +    if (aRhs.template is<N>()) {
> +      ::new (KnownNotNull, aLhs) T(aRhs.template extract<N>());
>      } else {
>        Next::moveConstruct(aLhs, aRhs);

Unrelated: this needs to be `moveConstruct(aLhs, Move(aRhs))`, doesn't it?  File a followup bug?

::: mfbt/Variant.h:268
(Diff revision 2)
> -    if (aLhs.template is<T>()) {
> -      MOZ_ASSERT(aRhs.template is<T>());
> -      return aLhs.template as<T>() == aRhs.template as<T>();
> +    if (aLhs.template is<N>()) {
> +      MOZ_ASSERT(aRhs.template is<N>());
> +      return aLhs.template as<N>() == aRhs.template as<N>();
>      } else {
>        return Next::equal(aLhs, aRhs);
>      }

Unrelated: these semantics seem unsafe; file a followup bug?

::: mfbt/Variant.h:556
(Diff revision 2)
>      return *this;
>    }
>  
>    /** Move assignment from AsVariant(). */
> -  template <typename T>
> -  Variant& operator=(detail::AsVariantTemporary<T>&& aValue)
> +  template<typename RefT,
> +           typename T = typename detail::SelectVariantType<RefT, Ts...>::Type>

What template magic is this parameter performing?  We're ensuring that we can find a type corresponding to the templated argument type?  Is this necessary for this patch, other patches in this same bug, or can this be relegating to a different bug, preferably with tests?
Attachment #8865366 - Flags: review?(nfroyd) → review+
Comment on attachment 8865367 [details]
Bug 1338389 - Allow repeated Variant types, but prevent is/as/extract<T> for them -

https://reviewboard.mozilla.org/r/137026/#review142578

I'd like to see the requested tests below, but the rest of this looks good.  I suppose if you wanted to roll the tests over into a followup bug, we could do that too.

::: mfbt/Variant.h:78
(Diff revision 2)
> -struct IsVariant<Needle, T, Haystack...> : public IsVariant<Needle, Haystack...> { };
> + * CountSelectableVariantTypes takes a type T and a list of variant types
> + * Variants and yields an integral constant count of how many type from
> + * Variants can store a value of type T or a reference to type T.

"CountSelectableVariantTypes takes a type T and a list of variant types Variants"?  Oh, I see.  Perhaps "...list of variant types, Variants, and..." would be clearer?

Nit: "...of how many type from..." should be "...of how many types from..."

::: mfbt/Variant.h:82
(Diff revision 2)
> +template <typename T, typename... Variants>
> +struct CountSelectableVariantTypes
> +  : public CountVariantTypeHelper<typename RemoveConst<typename RemoveReference<T>::Type>::Type,
> +                                  Variants...>

I think we should have some `static_assert` tests for this helper, as it's complicated enough.  It'd be nice if (some of) the code could be shared with `SelectVariantTypeHelper`, since they're both doing basically the same thing, but perhaps that can be a followup bug.

::: mfbt/Variant.h:392
(Diff revision 2)
> + *
> + *    // Bad!
> + *    template <typename T>
> + *    struct ResultOrError
> + *    {
> + *      Variant<R, int> m;

Shouldn't this be `Variant<T, int>`?

::: mfbt/Variant.h:405
(Diff revision 2)
> + *      Variant<R, int> m;
> + *      bool IsResult() const { return m.is<0>(); } // 0 -> R

Likewise here (though you used `.is<T>` above...renaming mix-up?).
Attachment #8865367 - Flags: review?(nfroyd) → review+
Comment on attachment 8865368 [details]
Bug 1338389 - VariantType<T> and VariantIndex<N> permit unambiguous and variadic Variant construction -

https://reviewboard.mozilla.org/r/137028/#review142580

Thanks for the improved documentation here.

::: mfbt/Variant.h:637
(Diff revision 2)
> -  template<typename RefT,
> +  template<typename RefT>
> -           typename T = typename detail::SelectVariantType<RefT, Ts...>::Type>

Please move this hunk to the first patch if at all possible.
Attachment #8865368 - Flags: review?(nfroyd) → review+
Comment on attachment 8865366 [details]
Bug 1338389 - Index-based Variant::is<N>, as<N>, and extract<N> -

https://reviewboard.mozilla.org/r/137024/#review142576

> Unrelated: this needs to be `moveConstruct(aLhs, Move(aRhs))`, doesn't it?  File a followup bug?

Good catch! I've opened bug 1365802.

> Unrelated: these semantics seem unsafe; file a followup bug?

I don't understand the issue here. What's unsafe?

> What template magic is this parameter performing?  We're ensuring that we can find a type corresponding to the templated argument type?  Is this necessary for this patch, other patches in this same bug, or can this be relegating to a different bug, preferably with tests?

I undid that in part 3!
I think my initial impulse was to be consistent with the constructor taking AsVariant.
(The "magic" was that it would just SFINAE-disappear that operator= if the T in AsVariant<T> was not in the Variant's type list; But after reflection it's better to keep it alive and produce a better error in that case.)
Comment on attachment 8865367 [details]
Bug 1338389 - Allow repeated Variant types, but prevent is/as/extract<T> for them -

https://reviewboard.mozilla.org/r/137026/#review142578

> I think we should have some `static_assert` tests for this helper, as it's complicated enough.  It'd be nice if (some of) the code could be shared with `SelectVariantTypeHelper`, since they're both doing basically the same thing, but perhaps that can be a followup bug.

It was easy enough to combine both, by adding to SelectVariantType a `static constexpr size_t count` that gives the total number of selectable types. (Note that IntegralConstant was not easily usable in this case, because it introduces its own `Type` definition that clashes with SelectVariantType::Type.)

Do we still need tests for the count? And if yes, shouldn't we also have some tests for the Type?
Thank you for the review.
Feel free to glance at my update, hopefully the new SelectVariantType::count makes sense.

And two questions please: (no hurry)

1.
(In reply to Nathan Froyd [:froydnj] from comment #17)
> Comment on attachment 8865366 [details]
> https://reviewboard.mozilla.org/r/137024/#review142576
> 
> ::: mfbt/Variant.h:268
> (Diff revision 2)
> > -    if (aLhs.template is<T>()) {
> > -      MOZ_ASSERT(aRhs.template is<T>());
> > -      return aLhs.template as<T>() == aRhs.template as<T>();
> > +    if (aLhs.template is<N>()) {
> > +      MOZ_ASSERT(aRhs.template is<N>());
> > +      return aLhs.template as<N>() == aRhs.template as<N>();
> >      } else {
> >        return Next::equal(aLhs, aRhs);
> >      }
> 
> Unrelated: these semantics seem unsafe; file a followup bug?

I don't understand the issue here. What's unsafe?

2.
(In reply to Nathan Froyd [:froydnj] from comment #18)
> Comment on attachment 8865367 [details]
> https://reviewboard.mozilla.org/r/137026/#review142578
> 
> ::: mfbt/Variant.h:82
> > +template <typename T, typename... Variants>
> > +struct CountSelectableVariantTypes
> > +  : public CountVariantTypeHelper<typename RemoveConst<typename RemoveReference<T>::Type>::Type,
> > +                                  Variants...>
> 
> I think we should have some `static_assert` tests for this helper, as it's
> complicated enough.  It'd be nice if (some of) the code could be shared with
> `SelectVariantTypeHelper`, since they're both doing basically the same
> thing, but perhaps that can be a followup bug.

As I've removed CountVariantType&Helper and instead just added a `count` member to the existing SelectVariantType, do we still need tests for this count? And if yes, shouldn't we also have some tests for the Type?
Flags: needinfo?(nfroyd)
(In reply to Gerald Squelart [:gerald] from comment #25)
> > ::: mfbt/Variant.h:268
> > (Diff revision 2)
> > > -    if (aLhs.template is<T>()) {
> > > -      MOZ_ASSERT(aRhs.template is<T>());
> > > -      return aLhs.template as<T>() == aRhs.template as<T>();
> > > +    if (aLhs.template is<N>()) {
> > > +      MOZ_ASSERT(aRhs.template is<N>());
> > > +      return aLhs.template as<N>() == aRhs.template as<N>();
> > >      } else {
> > >        return Next::equal(aLhs, aRhs);
> > >      }
> > 
> > Unrelated: these semantics seem unsafe; file a followup bug?
> 
> I don't understand the issue here. What's unsafe?

Assuming that if variant_typeof(aLhs) == T, then clearly variant_typeof(aRhs) == T seems wrong, especially if we're unsafely accessing aRhs as T in non-debug builds.  ISTM that the checks should be something more like:

  if (aLhs.template is<T>() && aRhs.template is<T>()) {
    ...
  } ...

but with some logic so that if we run out of types, we just return false.

> (In reply to Nathan Froyd [:froydnj] from comment #18)
> > Comment on attachment 8865367 [details]
> > https://reviewboard.mozilla.org/r/137026/#review142578
> > 
> > ::: mfbt/Variant.h:82
> > > +template <typename T, typename... Variants>
> > > +struct CountSelectableVariantTypes
> > > +  : public CountVariantTypeHelper<typename RemoveConst<typename RemoveReference<T>::Type>::Type,
> > > +                                  Variants...>
> > 
> > I think we should have some `static_assert` tests for this helper, as it's
> > complicated enough.  It'd be nice if (some of) the code could be shared with
> > `SelectVariantTypeHelper`, since they're both doing basically the same
> > thing, but perhaps that can be a followup bug.
> 
> As I've removed CountVariantType&Helper and instead just added a `count`
> member to the existing SelectVariantType, do we still need tests for this
> count? And if yes, shouldn't we also have some tests for the Type?

I think so, yes, on both counts.
Flags: needinfo?(nfroyd)
Back to this!

(In reply to Nathan Froyd [:froydnj] from comment #26)
> (In reply to Gerald Squelart [:gerald] from comment #25)
> > > ::: mfbt/Variant.h:268
> > > (Diff revision 2)
> > > > +    if (aLhs.template is<N>()) {
> > > > +      MOZ_ASSERT(aRhs.template is<N>());
> > > 
> > > Unrelated: these semantics seem unsafe; file a followup bug?
> [...]
> Assuming that if variant_typeof(aLhs) == T, then clearly
> variant_typeof(aRhs) == T seems wrong, especially if we're unsafely
> accessing aRhs as T in non-debug builds.  ISTM that the checks should be
> something more like:
> 
>   if (aLhs.template is<T>() && aRhs.template is<T>()) {
>     ...
>   } ...
> 
> but with some logic so that if we run out of types, we just return false.

I see now what you meant...

But if you look at the initial call in Variant::operator==(const Variant&),
- Both objects are of the exact same Variant type, so their contained types will be the same and in the same order.
- There is a first check whether both Variant object currently contain the same type before we call Impl::equal().

So when calling Impl::equal, the `if(is<N>)` is only used to go along the chain of types until we get to the correct type to compare; the MOZ_ASSERT is (I think) just there to ensure the tag check had already been done, and as implicit documentation that the following '==' is done on sub-objects of the same (contained) type.

So I believe adding the 2nd is<N> test would be redundant.


> > (In reply to Nathan Froyd [:froydnj] from comment #18)
> > As I've removed CountVariantType&Helper and instead just added a `count`
> > member to the existing SelectVariantType, do we still need tests for this
> > count? And if yes, shouldn't we also have some tests for the Type?
> 
> I think so, yes, on both counts.

Ok, will do.
Comment on attachment 8874713 [details]
Bug 1338389 - Tests for Variant's detail::Nth and detail::SelectVariantType -

https://reviewboard.mozilla.org/r/146092/#review150204

Thank you!
Attachment #8874713 - Flags: review?(nfroyd) → review+
Pushed by gsquelart@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d0254e59eb10
Index-based Variant::is<N>, as<N>, and extract<N> - r=froydnj
https://hg.mozilla.org/integration/autoland/rev/1a8ecb297f24
Allow repeated Variant types, but prevent is/as/extract<T> for them - r=froydnj
https://hg.mozilla.org/integration/autoland/rev/e84a5251008f
VariantType<T> and VariantIndex<N> permit unambiguous and variadic Variant construction - r=froydnj
https://hg.mozilla.org/integration/autoland/rev/0dec1e86d314
Tests for Variant's detail::Nth and detail::SelectVariantType - r=froydnj
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: