Simplify Variant::match API

RESOLVED FIXED in Firefox 48

Status

()

Core
JavaScript: GC
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: terrence, Assigned: terrence)

Tracking

Trunk
mozilla50
Points:
---

Firefox Tracking Flags

(firefox46 affected, firefox48 fixed, firefox50 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

2 years ago
Created attachment 8698455 [details] [diff] [review]
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.
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.
(Assignee)

Comment 2

2 years ago
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.
(Assignee)

Comment 3

2 years ago
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]
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!!
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).
(Assignee)

Comment 7

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

Comment 8

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

Comment 9

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e673dbc9848f
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
reopen and backed out since this broke VS 2013 Builds (and we still support this configuration).
Status: RESOLVED → REOPENED
Flags: needinfo?(terrence)
Resolution: FIXED → ---
Created attachment 8742023 [details]
local build bustage on vs2013
(Assignee)

Comment 15

2 years ago
Relanded! Took awhile to rebase because of all the new usage.
Flags: needinfo?(terrence)

Comment 16

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2ce26d5c3376
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: mozilla48 → mozilla50
You need to log in before you can comment on or make changes to this bug.