Closed
Bug 1255925
Opened 8 years ago
Closed 8 years ago
Give a name to getters/setters and integer-named methods
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla48
People
(Reporter: evilpie, Assigned: evilpie)
References
Details
Attachments
(4 files, 2 obsolete files)
21.14 KB,
patch
|
efaust
:
review+
|
Details | Diff | Splinter Review |
1.71 KB,
patch
|
fitzgen
:
review+
|
Details | Diff | Splinter Review |
26.56 KB,
patch
|
efaust
:
review+
|
Details | Diff | Splinter Review |
49.11 KB,
patch
|
gkw
:
feedback+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•8 years ago
|
||
Assignee | ||
Comment 2•8 years ago
|
||
This allows us to name methods in all cases excluding computed property names. As far as I can tell propAtom already contains something sensible in all cases that I could come up with. I had to change a bunch of functions to take HandleAtom instead of HandlePropertyName, because for {123(){}}, 123 is obviously not a property name ... I am not really happy having to call ConcatStrings inside the parser, but that seems at least better than somehow manually memcpying the atoms chars around.
Attachment #8729752 -
Attachment is obsolete: true
Attachment #8737921 -
Flags: review?(efaustbmo)
Comment 3•8 years ago
|
||
Comment on attachment 8737921 [details] [diff] [review] Give a function name to getter/setters/methods. Review of attachment 8737921 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me. The fact that they specced |Object.getOwnPropertyDescriptor({ get foo() { } }, "foo").get.name === "get foo"| is astonishing, but it does seem to be the case. ::: js/src/frontend/Parser.cpp @@ +2000,5 @@ > + RootedAtom prefix(context); > + if (propType == PropertyType::Setter || propType == PropertyType::SetterNoExpressionClosure) > + prefix = context->names().setPrefix; > + else > + prefix = context->names().getPrefix; Please add a MOZ_ASSERT in here somewhere about propType. @@ +2002,5 @@ > + prefix = context->names().setPrefix; > + else > + prefix = context->names().getPrefix; > + > + RootedString str(context, ConcatStrings<CanGC>(context, prefix, propAtom)); Would also love to assert somehow that propAtom is non-empty. ::: js/src/tests/js1_8_5/reflect-parse/classes.js @@ +10,5 @@ > + methodName = typeof id === 'string' ? ident(id) : null; > + break; > + case "get": > + case "set": > + methodName = ident(`${kind} ${typeof id === 'string' ? id : ""}`); nice.
Attachment #8737921 -
Flags: review?(efaustbmo) → review+
Assignee | ||
Comment 4•8 years ago
|
||
Thanks for reviewing. (In reply to Eric Faust [:efaust] from comment #3) > Comment on attachment 8737921 [details] [diff] [review] > @@ +2002,5 @@ > > + prefix = context->names().setPrefix; > > + else > > + prefix = context->names().getPrefix; > > + > > + RootedString str(context, ConcatStrings<CanGC>(context, prefix, propAtom)); > > Would also love to assert somehow that propAtom is non-empty. > We can't do that: ({get ""() {}}) would be empty.
Assignee | ||
Comment 5•8 years ago
|
||
Attachment #8740480 -
Flags: review?(nfitzgerald)
Assignee | ||
Comment 6•8 years ago
|
||
Because we set an explicit name for get/set now we don't get the inferred name from the analysis anymore.
Updated•8 years ago
|
Attachment #8740480 -
Flags: review?(nfitzgerald) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a5cf306560d https://hg.mozilla.org/integration/mozilla-inbound/rev/ff56abc1768b
Comment 8•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/0a5cf306560d https://hg.mozilla.org/mozilla-central/rev/ff56abc1768b
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Assignee | ||
Updated•8 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 9•8 years ago
|
||
I have to back this out. JSFunction::name returns a js::PropertyName, which is not ok anymore.
Comment 10•8 years ago
|
||
backed out on request from Tom on m-c per irc
Comment 11•8 years ago
|
||
Backout: https://hg.mozilla.org/mozilla-central/rev/d91b00cc6ec6 https://hg.mozilla.org/mozilla-central/rev/10f66b316457
Assignee | ||
Comment 12•8 years ago
|
||
This should fix that fuzzbug. I would appreciate to get some fuzzing for before I land this again.
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(gary)
Flags: needinfo?(choller)
Comment on attachment 8742053 [details] [diff] [review] WIP Change JSFunction::name to return a JSAtom I ran this quickly for a short time on Mac and it didn't seem to blow anything up.
Flags: needinfo?(gary)
Attachment #8742053 -
Flags: feedback+
Assignee | ||
Comment 14•8 years ago
|
||
Attachment #8743504 -
Flags: review?(efaustbmo)
Assignee | ||
Updated•8 years ago
|
Attachment #8742053 -
Attachment is obsolete: true
Assignee | ||
Comment 15•8 years ago
|
||
Attachment #8743801 -
Flags: feedback?(gary)
It is heavily preferred to list the m-c revision that the roll-up patch applies to, next time. In this case, it seems to apply to m-c rev 0891f0fa044c.
Comment on attachment 8743801 [details] [diff] [review] Rollup for fuzzing Nothing found specifically after few days' worth of fuzzing on a 64-bit debug deterministic js shell.
Attachment #8743801 -
Flags: feedback?(gary) → feedback+
Assignee | ||
Comment 18•8 years ago
|
||
Thanks for fuzzing! Eric feel free to review this now.
Flags: needinfo?(choller)
Comment 19•8 years ago
|
||
Comment on attachment 8743504 [details] [diff] [review] Change JSFunction::name to return a JSAtom Review of attachment 8743504 [details] [diff] [review]: ----------------------------------------------------------------- WFM. ::: js/src/frontend/Parser.cpp @@ +941,5 @@ > Parser<ParseHandler>::reportBadReturn(Node pn, ParseReportKind kind, > unsigned errnum, unsigned anonerrnum) > { > JSAutoByteString name; > + if (JSAtom* atom = pc->sc->asFunctionBox()->function()->name()) { This is gonna warn without another set of parens, I think.
Attachment #8743504 -
Flags: review?(efaustbmo) → review+
Comment 20•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/310418e4db4b https://hg.mozilla.org/integration/mozilla-inbound/rev/ac7e7e4f0b48 https://hg.mozilla.org/integration/mozilla-inbound/rev/a8f65fd17dc2
Comment 21•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/310418e4db4b https://hg.mozilla.org/mozilla-central/rev/ac7e7e4f0b48 https://hg.mozilla.org/mozilla-central/rev/a8f65fd17dc2
Status: REOPENED → RESOLVED
Closed: 8 years ago → 8 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•