1246838 - Handle const qualifiers and references better in Variant

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Seth Fowler [:seth] [:s2h]]

Currently Variant's handling of const qualifiers is pretty unintuitive. The way we decide if a type T can be stored in a Variant<V1, V2, ...> is by:

(1) Removing any const qualifier.
(2) Removing any reference qualifier.
(3) Checking for an exact match between T and one of V1, V2, etc.

This algorithm is not great. For |const X&|, we fail to match |X| because we remove the |const| and the |&| in the wrong order. That could be fixed by simplying swapping steps (1) and (2), but this breaks some existing code which relies on the current order to allow |Variant<const V1, V2, ...>| to work. Once we perform the swap, |const X&| gets stripped all the way down to |X| and no longer matches |Variant<const X, ...>|. So we unfortunately need to be a little smarter than that.

In this bug we'll replace the existing algorithm with a smarter one that

(1) Swaps step (1) and step (2) above.
(2) Walks the Variant subtypes and allows storing a |T| value in a |T|, |const T|, |const T&|, or |T&&| subtype.

This fixes the |const X&| problem explained above and makes existing code that uses the |Variant<const V1, ...>| construction continue to work.

Comment 1

[Seth Fowler [:seth] [:s2h]]

Attachment: Handle const qualifiers and references better in Variant.

Here's the patch. The solution discussed in comment 0 is implemented by
introducing a SelectVariantType helper template which walks the Variant subtypes
and performs the matching.

Comment 2

[Jeff Walden [:Waldo]]

Comment on attachment 8717264 [details] [diff] [review]
Handle const qualifiers and references better in Variant.

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

::: mfbt/Variant.h
@@ +66,5 @@
> +struct SelectVariantTypeHelper;
> +
> +template<typename T>
> +struct SelectVariantTypeHelper<T>
> +{ };

Just declare this -- if it's ever instantiated it'll be a compile error later, so no point defining IMO.

@@ +89,5 @@
> +
> +template<typename T, typename... Variants>
> +struct SelectVariantTypeHelper<T, T&&, Variants...>
> +{
> +  typedef T&& Type;

I think this wants to be just T -- that's what it is now, it makes sense to me that you'd shove a T&& temporary into a T so that *that* could then become a temporary by virtue of the containing Variant's behavior, and storing T&& references into struct fields seems dangerous and/or at the very least generally frowned upon.  Or am I missing something?

@@ +106,5 @@
> +template <typename T, typename... Variants>
> +struct SelectVariantType
> +  : public SelectVariantTypeHelper<typename RemoveConst<typename RemoveReference<T>::Type>::Type,
> +                                   Variants...>
> +{ };

Vague recollection of TypeTraits.h says we use "{}", if so use that.

Comment 3

[Seth Fowler [:seth] [:s2h]]

Thanks for the review!

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #2)
> I think this wants to be just T -- that's what it is now, it makes sense to
> me that you'd shove a T&& temporary into a T so that *that* could then
> become a temporary by virtue of the containing Variant's behavior, and
> storing T&& references into struct fields seems dangerous and/or at the very
> least generally frowned upon.  Or am I missing something?

We'd only hit that case if the user constructed a Variant<T&&>; all I'm doing here is allowing you to pass a T into a Variant that stores e.g. Variant<const T> or Variant<const T&> or Variant<T&&>. Do we use the type system to forbid Variant<T&&>? If so I can remove that specialization of the template, but if not I'm not sure that this is the place to enforce that.

Comment 4

[Seth Fowler [:seth] [:s2h]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #2)
> Vague recollection of TypeTraits.h says we use "{}", if so use that.

I think we don't use Gecko style in MFBT, so I'll change it, but FWIW Gecko style says we should use |{ }|:

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Classes

Comment 5

[Seth Fowler [:seth] [:s2h]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #2)
> ::: mfbt/Variant.h
> @@ +66,5 @@
> > +struct SelectVariantTypeHelper;
> > +
> > +template<typename T>
> > +struct SelectVariantTypeHelper<T>
> > +{ };
> 
> Just declare this -- if it's ever instantiated it'll be a compile error
> later, so no point defining IMO.

Doh, actually this doesn't work because SelectVariantType ends up being a subclass of this type since it's the recursive base case.

Comment 6

[Seth Fowler [:seth] [:s2h]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5e17c239a417

Comment 7

[JW Wang [:jwwang] [:jw_wang]]

(In reply to Seth Fowler [:seth] [:s2h] from comment #3)
> We'd only hit that case if the user constructed a Variant<T&&>; all I'm
> doing here is allowing you to pass a T into a Variant that stores e.g.
> Variant<const T> or Variant<const T&> or Variant<T&&>. Do we use the type
> system to forbid Variant<T&&>? If so I can remove that specialization of the
> template, but if not I'm not sure that this is the place to enforce that.

Variant<int&&> gave me compiler errors like |forming pointer to reference type ‘int&&’|.

Comment 8

[Nicholas Nethercote [inactive]]

(In reply to Seth Fowler [:seth] [:s2h] from comment #4)
> 
> I think we don't use Gecko style in MFBT

Oh yes we do! With two tiny exceptions. See mfbt/STYLE.

Comment 9

[JW Wang [:jwwang] [:jw_wang]]

  struct Foo {
    Foo() {
      printf("Foo default ctor\n");
    }
    Foo(const Foo&) {
      printf("Foo copy ctor\n");
    }
    Foo(Foo&&) {
      printf("Foo move ctor\n");
    }
  };

  mozilla::Variant<const Foo> v(Foo {});

output:
Foo default ctor
Foo copy ctor

It looks wrong to me because move constructor should be called instead the copy constructor.

Comment 10

[Jeff Walden [:Waldo]]

(In reply to Seth Fowler [:seth] [:s2h] from comment #3)
> We'd only hit that case if the user constructed a Variant<T&&>; all I'm
> doing here is allowing you to pass a T into a Variant that stores e.g.
> Variant<const T> or Variant<const T&> or Variant<T&&>. Do we use the type
> system to forbid Variant<T&&>? If so I can remove that specialization of the
> template, but if not I'm not sure that this is the place to enforce that.

Ah, yeah, I think you're right.  CARRY ON.

(In reply to JW Wang [:jwwang] from comment #9)
>   struct Foo {
>     Foo() {
>       printf("Foo default ctor\n");
>     }
>     Foo(const Foo&) {
>       printf("Foo copy ctor\n");
>     }
>     Foo(Foo&&) {
>       printf("Foo move ctor\n");
>     }
>   };
> 
>   mozilla::Variant<const Foo> v(Foo {});
> 
> output:
> Foo default ctor
> Foo copy ctor
> 
> It looks wrong to me because move constructor should be called instead the
> copy constructor.

Yeah.  I think this is a quasi-pre-existing problem.  This:

  new (ptr()) T(Forward<T>(aT));

should really be

  new (ptr()) T(Forward<RefT>(aT));

It's nearly always wrong in practice to have Forward<T>(t) where the enclosing method doesn't have |template <typename T>| and its arguments don't contain |T&& t|.  This looks like an instance of that, as it exists now.

Comment 11

[Seth Fowler [:seth] [:s2h]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #10)
> Yeah.  I think this is a quasi-pre-existing problem.  This:
> 
>   new (ptr()) T(Forward<T>(aT));
> 
> should really be
> 
>   new (ptr()) T(Forward<RefT>(aT));
> 
> It's nearly always wrong in practice to have Forward<T>(t) where the
> enclosing method doesn't have |template <typename T>| and its arguments
> don't contain |T&& t|.  This looks like an instance of that, as it exists
> now.

This looks right to me, just from eyeballing it. I'll write up a patch to fix this as long as I'm fiddling around with Variant code.

Comment 12

[Seth Fowler [:seth] [:s2h]]

(In reply to Nicholas Nethercote [:njn] from comment #8)
> (In reply to Seth Fowler [:seth] [:s2h] from comment #4)
> > 
> > I think we don't use Gecko style in MFBT
> 
> Oh yes we do! With two tiny exceptions. See mfbt/STYLE.

Looks like my knowledge is out of date. Thanks Nick.

Comment 13

[Seth Fowler [:seth] [:s2h]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a48d1a9ea9063396b6600357a2b5014274f2d777
Bug 1246838 - Handle const qualifiers and references better in Variant. r=waldo

Comment 14

[JW Wang [:jwwang] [:jw_wang]]

It might be a plus to also support volatile qualifiers in the future.

Comment 15

[Nathan Froyd [:froydnj]]

(In reply to JW Wang [:jwwang] from comment #14)
> It might be a plus to also support volatile qualifiers in the future.

What would be the benefit to this?  I should think that any use of |volatile| in Gecko code almost certainly indicates a bug.

Comment 16

[JW Wang [:jwwang] [:jw_wang]]

Just to make it complete. If use of volatile is a bug, we should prevent something like IsVolatile<> in the first place though.

Comment 17

[Jeff Walden [:Waldo]]

Eh, whatever.  If someone uses volatile, almost certainly wrongly, they'll complain then and we can deal then.  Not worth the effort to do anything now.

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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