Closed
Bug 1232686
Opened 9 years ago
Closed 8 years ago
Simplify Variant::match API
Categories
(Core :: JavaScript: GC, defect)
Core
JavaScript: GC
Tracking
()
RESOLVED
FIXED
mozilla50
People
(Reporter: terrence, Assigned: terrence)
References
Details
Attachments
(2 files)
9.48 KB,
patch
|
fitzgen
:
review+
|
Details | Diff | Splinter Review |
6.31 KB,
text/plain
|
Details |
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)
Comment 1•9 years ago
|
||
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•9 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•9 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 4•9 years ago
|
||
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+
Updated•9 years ago
|
Summary: Simplify Varient::match API → Simplify Variant::match API
Comment 5•9 years ago
|
||
(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 6•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c21cc3ff9f77
Assignee | ||
Comment 7•8 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•8 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•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e673dbc9848f
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Comment 10•8 years ago
|
||
reopen and backed out since this broke VS 2013 Builds (and we still support this configuration).
Status: RESOLVED → REOPENED
Flags: needinfo?(terrence)
Resolution: FIXED → ---
Comment 11•8 years ago
|
||
Assignee | ||
Comment 12•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cd65401d0f95
Assignee | ||
Comment 13•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=38f280cc3014
Assignee | ||
Comment 14•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ce26d5c3376c756f2cfa43a4b75378879363a24 Bug 1232686 - Use decltype to infer Variant::match return type; r=fitzgen
Assignee | ||
Comment 15•8 years ago
|
||
Relanded! Took awhile to rebase because of all the new usage.
Flags: needinfo?(terrence)
Comment 16•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/2ce26d5c3376
Status: REOPENED → RESOLVED
Closed: 8 years ago → 8 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.
Description
•