Support passing rvalue matchers into mozilla::Variant::match

RESOLVED FIXED in Firefox 48

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: fitzgen, Assigned: fitzgen)

Tracking

unspecified
mozilla48
Points:
---

Firefox Tracking Flags

(firefox48 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

We have two versions right now: one for `const Matcher&` and one for `Matcher&` and I think we can combine them into one version.

Additionally, if we make it `Matcher&&`, the existing cases already work because Matcher is a template type parameter so it works similar to perfect forwarding except we aren't doing any constructing, and we can also do

  v.match(SomeMatcher());

instead of

  SomeMatcher m;
  v.match(m);

which we have to do now and is pretty annoying.
See Also: → 1254453
So `Matcher&& aMatcher` didn't work but `Matcher aMatcher` does. Will this
prefer passing by reference and avoiding copies when either could happen?
Attachment #8728183 - Flags: review?(nfroyd)
Summary: Clean up mozilla::Variant::match → Support passing rvalue matchers into mozilla::Variant::match
(In reply to Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] from comment #1)
> So `Matcher&& aMatcher` didn't work
How didn't it work? Doesn't perfect forwarding kick in in this case?
(In reply to JW Wang [:jwwang] from comment #3)
> (In reply to Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] from comment #1)
> > So `Matcher&& aMatcher` didn't work
> How didn't it work? Doesn't perfect forwarding kick in in this case?

That was what I had thought/hoped would happen, but the compiler did not agree. I don't remember the specific message anymore, but you could certainly try it locally.
template<typename Matcher, typename ConcreteVariant,
         typename ReturnType = typename RemoveReference<Matcher>::Type::ReturnType>
static ReturnType
match(Matcher&& aMatcher, ConcreteVariant& aV) {
  return aMatcher.match(aV.template as<T>());
}

RemoveReference needs to be applied to Matcher so ReturnType can be deducted correctly.
Awesome, thanks! I will attach a new patch in a bit :)
Comment on attachment 8728183 [details] [diff] [review]
Allow passing matchers as rvalues to Variant::match

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

Sounds like Nick's going to put up a new patch, so I'm going to get this one out of my queue. :)
Attachment #8728183 - Flags: review?(nfroyd)
Attachment #8728183 - Attachment is obsolete: true
Comment on attachment 8728517 [details] [diff] [review]
Allow passing matchers as rvalues to Variant::match

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

I am a bit surprised this takes so little code.  r=me, but I am interested to hear your thoughts on the suggestions below.

::: mfbt/Variant.h
@@ +553,5 @@
>      MOZ_ASSERT(is<T>());
>      return T(Move(as<T>()));
>    }
>  
> +  // Exhaustive matching of all variant types on the contained value.

What, no separate patch for this?? ;)

@@ +558,5 @@
>  
>    /** Match on an immutable const reference. */
> +  template<typename Matcher,
> +           typename ReturnType = typename RemoveReference<Matcher>::Type::ReturnType>
> +  ReturnType

I do like naming things better, but I wonder if we'd be better off not exposing this as an actual template parameter, but just sucking it up and saying:

template<typename Matcher>
typename RemoveReference<Matcher>::Type::ReturnType
match(Matcher&& aMatcher)

so as to not potentially imply to people that they could provide their own ReturnType, were they so inclined to be clever.  I'm less concerned about the usages in VariantImplementation, as those are internal details.

Also not sure if it's worth defining a separate helper template structure so as to not be saying RemoveReference<Matcher>::Type::ReturnType everywhere.
Attachment #8728517 - Flags: review?(nfroyd) → review+
Removed the type parameter for the public, non-detail changes and inlined the
whole expression instead. Good call.

Carrying r+.
Attachment #8729191 - Flags: review+
Attachment #8728517 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/4dccba6aa5a2
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.