Closed
Bug 1048384
Opened 10 years ago
Closed 10 years ago
Getter/setter syntax should work with computed property names
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla34
People
(Reporter: gupta.rajagopal, Assigned: gupta.rajagopal)
References
(Blocks 1 open bug)
Details
(Keywords: dev-doc-complete, Whiteboard: [DocArea=JS])
Attachments
(1 file, 2 obsolete files)
9.12 KB,
patch
|
gupta.rajagopal
:
review+
|
Details | Diff | Splinter Review |
a = {get [expr]() { return 3; }, set[expr](v) { return 2; }} should work, and not throw a syntax error.
Assignee: nobody → gupta.rajagopal
Status: NEW → ASSIGNED
Attachment #8470315 -
Flags: review?(jorendorff)
Comment 2•10 years ago
|
||
Comment on attachment 8470315 [details] [diff] [review] Patch to make getter/setter work with computed property names v1 Review of attachment 8470315 [details] [diff] [review]: ----------------------------------------------------------------- Nice! It might be fun to search existing tests for the use of std_iterator and switch them over to use the new syntax, where that's reasonable. Follow-up bug if so. ::: js/src/frontend/Parser.cpp @@ +7221,5 @@ > } > > template <typename ParseHandler> > +bool > +Parser<ParseHandler>::handleCompdPropName(Node literal, Node &propname) Please rename this to computedPropertyName. Also please change this method to return a Node, like all the other parsers (see objectLiteral(), for instance). If it fails, it should return null(); @@ +7320,5 @@ > if (!propname) > return null(); > + } else if (tt == TOK_LB) { > + // Computed property name. > + if (!handleCompdPropName(literal, propname)) With the renaming, the comment becomes redundant; remove it. ::: js/src/frontend/Parser.h @@ +618,5 @@ > Node pushLexicalScope(Handle<StaticBlockObject*> blockObj, StmtInfoPC *stmt); > Node pushLetScope(Handle<StaticBlockObject*> blockObj, StmtInfoPC *stmt); > bool noteNameUse(HandlePropertyName name, Node pn); > Node objectLiteral(); > + bool handleCompdPropName(Node literal, Node &propname); computedPropertyName ::: js/src/tests/ecma_6/Class/compPropNames.js @@ +204,5 @@ > +syntaxError("function f() { set [x](a): 1 }"); > +f1 = "abc"; > +syntaxError('a = {get [f1@](){}, set [f1](a){}}'); // unexpected symbol at end of AssignmentExpression > +syntaxError('a = {get@ [f1](){}, set [f1](a){}}'); // unexpected symbol after get > +syntaxError('a = {get [f1](){}, set@ [f1](a){}}'); // unexpected symbol after set beautiful negative tests :) @@ +214,5 @@ > + a.hey = 5; > +} catch (e) { > + if (e != 2) > + throw new Error('Expected 2 to be thrown on set'); > +} assertThrowsValue(() => { a.hey = 5; }, 2); The test code you wrote here wouldn't fail if a.hey = 5 silently passed without throwing. @@ +226,5 @@ > + a[expr] = 7; > +} catch (e) { > + if (e != 4) > + throw new Error('Expected 4 to be thrown on set'); > +} Please sanity-check that: - expressions with side effects are called in the right order, for example log = ""; obj = { "a": log += 'a', get [log += 'b']() {} [log += 'c']: log += 'd', set [log += 'e']() {} }; assertEq(log, "abcde"); - assignment expressions are allowed in the brackets - {} in the brackets is treated as an object literal - ({[/x/.source]: 0}) works (the /x/ is recognized as a regexp)
Attachment #8470315 -
Flags: review?(jorendorff) → review+
Addressed review comments.
Attachment #8470315 -
Attachment is obsolete: true
Attachment #8472687 -
Flags: review+
Comment 4•10 years ago
|
||
Guptha, is anything more needed for this to land, or can it be checked in?
Flags: needinfo?(gupta.rajagopal)
Merged. https://tbpl.mozilla.org/?tree=Try&rev=d8816fa5f641 Till, I'm on vacation and may not be able to follow up on the try run very promptly.
Attachment #8472687 -
Attachment is obsolete: true
Attachment #8477899 -
Flags: review+
Comment 6•10 years ago
|
||
(In reply to guptha from comment #5) > Till, I'm on vacation and may not be able to follow up on the try run very > promptly. Oh, sorry - I certainly didn't mean to interrupt your vacation. :(
(In reply to Till Schneidereit [:till] from comment #6) > Oh, sorry - I certainly didn't mean to interrupt your vacation. :( No problem!
Flags: needinfo?(gupta.rajagopal)
Keywords: checkin-needed
Comment 8•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb96975c7681
Flags: in-testsuite+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/cb96975c7681
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Updated•10 years ago
|
Keywords: dev-doc-needed
Whiteboard: [DocArea=JS]
Comment 10•10 years ago
|
||
Updated docs: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/get https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/set https://developer.mozilla.org/en-US/Firefox/Releases/34#JavaScript Reviews appreciated!
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•