Closed Bug 1429613 Opened 2 years ago Closed 9 months ago

Variant::match accepting lambdas, or one generic lambda

Categories

(Core :: MFBT, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla68
Tracking Status
firefox59 --- wontfix
firefox68 --- fixed

People

(Reporter: gerald, Assigned: jorendorff)

References

(Depends on 1 open bug)

Details

Attachments

(3 files, 1 obsolete file)

Currently the recommended way to handle all possibly options of a Variant is through the Variant::match() function, which takes a single struct that must contain a `match(T&)` function for each option.
This can be a bit heavy and difficult to read, separating the Variant::match call from the actions:
> Variant<A,B,C> vabc;
> struct Matcher
> {
>   char* match(A&) {...}
>   char* match(B&) {...}
>   char* match(C&) {...}
> };
> vabc.match(Matcher());


It would be great if instead we could call a matcher function directly with a bunch of lambdas, e.g.:
> vabc.fmatch(
>   [](A&) {...},
>   [](B&) {...},
>   [](C&) {...});


It could also accept one C++14 generic lambda, e.g.:
> vabc.fmatch([](auto&&) {...});
Blocks: 1429616
Seth and/or fitzgen talked about this when Variant was first introduced, but it wasn't really practical then.  I'd be in favor of supporting multiple lambdas, which doesn't seem *too* hard.  I think boost::variant supported multiple lambdas in any order (!), but let's require the lambdas to be matched in the order of the types declared in the Variant.  If people really don't like that, we can try the more complicated scheme.

Bonus points would be converting .match() to the lambda form everywhere, which is probably clearer.
Priority: -- → P3
Depends on: 1487933

Rebasing this stack. One patch done and building. Easy going so far.

Assignee: nobody → jorendorff

Mechanical change from Matcher::match(...) to Matcher::operator()(...).
This will now permit the use of generic lambdas, and facilitate the
implementation of multi-lambda match.

Mmmmaybe land this?

I spent a couple hours yesterday morning implementing a Variant::fmatch method template from scratch, very similar to this stack only without the renaming and otherwise worse in a couple ways. Then I searched Bugzilla.

Attachment #9053619 - Attachment description: Bug 1429613 - Add `Variant<Ts...>::fmatch(F...)`, an alternative to match() that takes lambdas. → (alternate approach, for the record) Bug 1429613 - Add `Variant<Ts...>::fmatch(F...)`, an alternative to match() that takes lambdas.

The patch uploaded in comment 8 is how it would look the other way (adding .fmatch() without changing the existing .match() template), stealing a few good ideas from Gerald's patches.

I like Gerald's stack better anyway.

Pushed by jorendorff@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5753c98c39d1
Variant matcher callbacks renamed from `match` to `operator()` - r=froydnj
https://hg.mozilla.org/integration/autoland/rev/700ec51653ee
Variadic Variant::match, takes one function object per option - r=froydnj
https://hg.mozilla.org/integration/autoland/rev/f88e8b806783
Using upgraded Variant::match where appropriate - r=froydnj
Status: NEW → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
Attachment #9053619 - Attachment description: (alternate approach, for the record) Bug 1429613 - Add `Variant<Ts...>::fmatch(F...)`, an alternative to match() that takes lambdas. → Bug 1429613 - Add `Variant<Ts...>::fmatch(F...)`, an alternative to match() that takes lambdas.
Attachment #9053619 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.