Closed Bug 1232686 Opened 5 years ago Closed 5 years ago

Simplify Variant::match API


(Core :: JavaScript: GC, defect)

Not set



Tracking Status
firefox46 --- affected
firefox48 --- fixed
firefox50 --- fixed


(Reporter: terrence, Assigned: terrence)




(2 files)

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.
Attachment #8698455 - Flags: review?(nfitzgerald)
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.
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.
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 on attachment 8698455 [details] [diff] [review]

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!!
Attachment #8698455 - Flags: review?(nfitzgerald) → review+
Summary: Simplify Varient::match API → Simplify Variant::match API
(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).
Took a bit of rebasing, but it seems that this patch doesn't crash on windows since we've updated the compiler there.
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
reopen and backed out since this broke VS 2013 Builds (and we still support this configuration).
Flags: needinfo?(terrence)
Resolution: FIXED → ---
Relanded! Took awhile to rebase because of all the new usage.
Flags: needinfo?(terrence)
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: mozilla48 → mozilla50
You need to log in before you can comment on or make changes to this bug.