Handle const qualifiers and references better in Variant

RESOLVED FIXED in Firefox 47

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: seth, Assigned: seth)

Tracking

unspecified
mozilla47
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 fixed)

Details

Attachments

(1 attachment)

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.
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.
Attachment #8717264 - Flags: review?(jwalden+bmo)
Blocks: 1246841
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.
Attachment #8717264 - Flags: review?(jwalden+bmo) → review+
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.
(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
(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.
(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&&’|.
(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.
  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.
(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.
(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.
(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.
Blocks: 1250666
https://hg.mozilla.org/integration/mozilla-inbound/rev/a48d1a9ea9063396b6600357a2b5014274f2d777
Bug 1246838 - Handle const qualifiers and references better in Variant. r=waldo
It might be a plus to also support volatile qualifiers in the future.
(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.
Just to make it complete. If use of volatile is a bug, we should prevent something like IsVolatile<> in the first place though.
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.
https://hg.mozilla.org/mozilla-central/rev/a48d1a9ea906
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.