1232686 - Simplify Variant::match API

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Terrence Cole [:terrence]]

Attachment: simplify_Variant_match_API-v0.diff

In the GC we have a type dispatch method for Value, jsid, GCCellPtr, and void*+TraceKind that is very similar to that used by Variant::match... but not quite. This make is so that we have to duplicate all of our existing GC functors in order to use GC types with Variant. I'd like to reify our dispatch methods so that we can share this code.

The first part is in how we define the return type from match. Currently, Variant passes this explicitly via a ReturnType in the matcher. When doing our own typed functor dispatch, Jon found that by combining C++11 decltype and auto, we can infer the return type directly from the method signatures on the given matcher. This allows us to drop the explicit ReturnType declaration from all the matchers.

Comment 1

[Jon Coppeard (:jonco)]

Looking at this again, that decltype business doesn't look too great but it's only used in a couple of places so it's probably OK.  What we really want here is C++14's return type deduction which I think will let us write |auto| and be done with it.

Comment 2

[Terrence Cole [:terrence]]

Yup, just verified locally: building with gnu++14 on clang 3.5.0 allows us to (1) drop the ->decltype(insanity) and (2) pass generic lambdas with auto. This allows us, for example, to do:

    JSCompartment* comp = DispatchTyped([](auto t) { return t->maybeCompartment(); }, thing);

With this, I think all of our functors become more or less trivial. Given that it's mostly going to be call dispatch, I think we don't even want to name them or bother commoning them up.

Comment 3

[Terrence Cole [:terrence]]

Note: the required compiler versions for us to switch to c++14 and do the above are: MSVC 2015, clang 3.4, and gcc 4.9. I guess the hard part is MSVC 2015 and b2g, currently using gcc 4.7. I think people are already working towards MSVC 2015; I don't know the state of b2g compiler support.

Comment 4

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

Comment on attachment 8698455 [details] [diff] [review]
simplify_Variant_match_API-v0.diff

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

LGTM, this is actually what I originally coded up for Variant, but I kept ICEing msvc and gave up with Matcher::ReturnType. If you can get a good try push and/or upgrade msvc, then Ship It!!

Comment 5

[Emanuel Hoogeveen [:ehoogeveen]]

(In reply to Terrence Cole [:terrence] from comment #3)
> I guess the hard part is MSVC 2015 and b2g, currently using gcc 4.7. I think
> people are already working towards MSVC 2015; I don't know the state of b2g
> compiler support.

IIRC B2G uses a custom build of GCC 4.8 for Gecko right now on v2.2 and up (as of bug 1056337), with bug 1087161 open to switch to GCC 4.9. I'm not sure how it works exactly - I think some parts of the build still require an older GCC, but I don't think it affects Gecko. The switch to 4.9 looks to have stalled for the moment though (but I think *most* of the hard work was done in switching to 4.8).

Comment 6

[Terrence Cole [:terrence]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c21cc3ff9f77

Comment 7

[Terrence Cole [:terrence]]

Took a bit of rebasing, but it seems that this patch doesn't crash on windows since we've updated the compiler there.

Comment 8

[Terrence Cole [:terrence]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e673dbc9848f2ae4cd11911e7b9fae8e4757cd0e
Bug 1232686 - Use decltype to infer Variant::match return type; r=fitzgen

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e673dbc9848f

Comment 10

[Carsten Book [:Tomcat]]

reopen and backed out since this broke VS 2013 Builds (and we still support this configuration).

Comment 11

[Carsten Book [:Tomcat]]

Attachment: local build bustage on vs2013

Comment 12

[Terrence Cole [:terrence]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cd65401d0f95

Comment 13

[Terrence Cole [:terrence]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=38f280cc3014

Comment 14

[Terrence Cole [:terrence]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2ce26d5c3376c756f2cfa43a4b75378879363a24
Bug 1232686 - Use decltype to infer Variant::match return type; r=fitzgen

Comment 15

[Terrence Cole [:terrence]]

Relanded! Took awhile to rebase because of all the new usage.

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2ce26d5c3376