1487933 - Make Variant::match call operator()(actual stored value) on the argument passed to it, not match(actual stored value)

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Jeff Walden [:Waldo]]

|Variant::match| is currently

  Variant::match(Matcher m)

and then returns |m.match(stored value)|.  This is nice for readability in that you're looking for "match", not random punctuation like (), to understand just how the matcher object is used.  And as the matcher you pass in almost necessarily must understand multiple types to pass to |m.match|, you're going to have to define the type of |m| in some other place in code -- and a word like "match" makes more obvious what places those are.

This logic carries far less force in the C++14 world when lambdas can be generic.  For sufficiently regular matching-functionality, you *can* define how to match inline, using generic lambdas.

So for example, we have this rather-verbose mess right now:

  struct ParseHandlerMatcher
  {
      template<class Parser>
      frontend::FullParseHandler& match(Parser *parser) {
        return parser->handler;
      }
  };

  class EitherParser : public BCEParserHandle
  {
      mozilla::Variant<Parser<FullParseHandler, char16_t>* const> parser;

    public:
      FullParseHandler& astGenerator() final {
        return parser.match(detail::ParseHandlerMatcher());
      }

      ...
  };

But suppose |Variant::match| called |operator()| on the argument passed to it, providing the properly-typed internal value.  Then we could do this instead:

  class EitherParser : public BCEParserHandle
  {
      mozilla::Variant<Parser<FullParseHandler, char16_t>* const> parser;

    public:
      FullParseHandler& astGenerator() final {
        return parser.match([](auto* parser) { return parser->handler; });
      }

      ...
  };

It feels to me like the win of being able to pass in generic lambdas in some cases, outweighs the loss of not being able to see "match" in the matcher-code in the rest.

But it is also possible that in most cases you're going to have sufficiently syntactically different match-operations, that you can't use generic lambdas.  So while this change would help out js/src/frontend/EitherParser.h, it would help out almost nothing else, and so the change wouldn't be worth it.

Thoughts?

(Another option would be to use operator() if it's defined but fall back to match() if operator() isn't around.  I imagine this is possible with enough SFINAE-style hackery.  My gut says that's way too magical, but I could be wrong about that.)

Comment 1

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

You have my vote for `operator()` -- for whatever it's worth :-)

Looks like I wanted to do that in bug 1429613, see:
https://hg.mozilla.org/try/rev/0ffe057c61973f9d2486e45c2b46b7826e3e99b8
Sorry I didn't finish it, because my need for it evaporated.
Feel free to steal it if it's not too stale.

I also have a local follow-up patch with a variadic match() that takes one lambda per Variant option, it allows folding the matcher struct into the Variant::match call, for better overall clarity (I think) and possible compiler optimization (I hope). Let me know if you'd like that too, otherwise you/I/someone can always do that later in bug 1429613 as originally intended.

Good luck!

Comment 2

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

I just couldn't leave it alone, could I? :-D So here's a rebased patch:
https://hg.mozilla.org/try/rev/2dac4335b79d54aec34f5125be6f0f8ea4afeb1f

It's from this try, which also implements variadic match() :
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ba04e11b05b88aa7e7910eb648be7bfd00982551
The 3rd patch has some generic-lambda examples that could be used here too.

Comment 3

[Nathan Froyd [:froydnj]]

(In reply to Jeff Walden [:Waldo] from comment #0)
> It feels to me like the win of being able to pass in generic lambdas in some
> cases, outweighs the loss of not being able to see "match" in the
> matcher-code in the rest.
> 
> But it is also possible that in most cases you're going to have sufficiently
> syntactically different match-operations, that you can't use generic
> lambdas.  So while this change would help out
> js/src/frontend/EitherParser.h, it would help out almost nothing else, and
> so the change wouldn't be worth it.

My sense is that this is more often the case: your types in Variant<> objects should usually not be related so tightly that generic lambdas are practical to use.  And if they are, I think that is the exception that proves the rule?

> Thoughts?
> 
> (Another option would be to use operator() if it's defined but fall back to
> match() if operator() isn't around.  I imagine this is possible with enough
> SFINAE-style hackery.  My gut says that's way too magical, but I could be
> wrong about that.)

Supporting lambdas in Variant::match seems reasonable, so people don't have to write separate match objects (and lose on capturing, etc.).  But I have no desire to write that code. ;)

Are those two opinions conflicting?

Comment 4

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.

Comment 5

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

This was effectively implemented in bug 1429613, landed in Firefox 68.