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

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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.

Comment 1

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Allow passing matchers as rvalues to Variant::match

So `Matcher&& aMatcher` didn't work but `Matcher aMatcher` does. Will this
prefer passing by reference and avoiding copies when either could happen?

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e3e92af0f17d

Comment 3

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

(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?

Comment 4

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(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.

Comment 5

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

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.

Comment 6

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Awesome, thanks! I will attach a new patch in a bit :)

Comment 7

[Nathan Froyd [:froydnj]]

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. :)

Comment 8

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Allow passing matchers as rvalues to Variant::match

Comment 9

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8c5999003024

Comment 10

[Nathan Froyd [:froydnj]]

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.

Comment 11

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Allow passing matchers as rvalues to Variant::match

Removed the type parameter for the public, non-detail changes and inlined the
whole expression instead. Good call.

Carrying r+.

Comment 12

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4dccba6aa5a2

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4dccba6aa5a2