Implement support for |{ get "string literal"() { /* ... */ }, get 5.4() { /* ... */ }, 6.72: 3 }|

RESOLVED FIXED in mozilla1.9.3a1

Status

()

Core
JavaScript Engine
--
enhancement
RESOLVED FIXED
8 years ago
6 years ago

People

(Reporter: Waldo, Assigned: Waldo)

Tracking

({dev-doc-complete})

Trunk
mozilla1.9.3a1
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

ES5's rules for object literals:

ObjectLiteral :
      { }
      { PropertyNameAndValueList }
      { PropertyNameAndValueList , }
PropertyNameAndValueList :
      PropertyAssignment
      PropertyNameAndValueList , PropertyAssignment
PropertyAssignment :
      PropertyName : AssignmentExpression
      get PropertyName ( ) { FunctionBody }
      set PropertyName ( PropertySetParameterList ) { FunctionBody }
PropertyName :
      IdentifierName
      StringLiteral
      NumericLiteral
PropertySetParameterList :
      Identifier

We support |PropertyName: AssignmentExpression|, and we support |[gs]et IdentifierName|, but we don't support |[gs]et (String|Numeric)Literal|.  Needs to happen to be able to get rid of |x getter:| syntax.
Note that NumericLiteral is any number literal, possibly fractional, possibly exponential (even with positive or negative exponent), not just DecimalIntegerLiteral.
Blocks: 517580
...which means we also have to handle { 3.141592654: "π" }.
Summary: Implement support for |{ get "string literal"() { /* ... */ }, get 5() { /* ... */ } }| → Implement support for |{ get "string literal"() { /* ... */ }, get 5.4() { /* ... */ }, 6.72: 3 }|
Created attachment 417717 [details] [diff] [review]
Patch

Hm, so we already supported non-DecimalInteger numeric literals as property names (if not as getter/setter names).  That makes things a little simpler.
Attachment #417717 - Flags: review?(jim)

Comment 4

7 years ago
Curious --- why the initialization of funAtom in FunctionDef?  That's definitely set by the first 'if'; was some compiler too dumb to see that?

Comment 5

7 years ago
(In reply to comment #3)
> Created an attachment (id=417717) [details]
> Patch
> 
> Hm, so we already supported non-DecimalInteger numeric literals as property
> names (if not as getter/setter names).  That makes things a little simpler.

Yeah; that's tested in js/src/tests/ecma_5/strict/11.1.5.js.

Comment 6

7 years ago
Shouldn't those tests go in js/src/tests/ecma_5/Expressions/11.1.5.js?  We're just implementing what the standard specifies.
(In reply to comment #4)
> why the initialization of funAtom in FunctionDef?

I think that's a slight bit of patch-cest, anticipating a change much later in my mq where I make |{ set foo foo() { } }| (you don't really want to know) a syntax error.
(In reply to comment #6)
> Shouldn't those tests go in js/src/tests/ecma_5/Expressions/11.1.5.js?

I don't see an ecma_5/Expressions directory in TM or m-c.

(And to be clear, I can remove the patch-cest if desired, don't think it hurts as is tho.)
What's a patch-cest? If it's what it sounds like, the taboo against it stands :-/.

/be
Created attachment 424883 [details] [diff] [review]
Updated

That hunk more properly fit in the patch atop this one in my mq.  I've moved it out of here and, incidentally, moved the declaration nearer to point of first use; it's probably better moving anyway.
Attachment #417717 - Attachment is obsolete: true
Attachment #424883 - Flags: review?(jim)
Attachment #417717 - Flags: review?(jim)
Yes, please do move declarations to nearest dominating position and initialize 'em -- the old C heritage with a thicket of uninitialized vars makes it easy to miss an initializer (not to mention a var).

/be

Comment 12

7 years ago
(In reply to comment #9)
> What's a patch-cest? If it's what it sounds like, the taboo against it stands
> :-/.

Little bits like whitespace cleanup I've decided not to complain

Comment 13

7 years ago
[koff koff] ... about because otherwise they'd never get done; no reason to douse someone's enthusiasm by making them do a bunch of dotting and crossing.

But stuff with semantic significance... no cest is best.

Comment 14

7 years ago
(In reply to comment #8)
> (In reply to comment #6)
> > Shouldn't those tests go in js/src/tests/ecma_5/Expressions/11.1.5.js?
> 
> I don't see an ecma_5/Expressions directory in TM or m-c.

Yeah, we'll need to make one.
Frankly, I don't think much of sub-organizing tests where distinctions don't map to source directories (as with SpiderMonkey where ~everything's in js/src/*); it just results in angst-fests about where they should go.  Even the ecma_5 and friends (especially js1_*) distinction is lost on me -- JS is usually just JS, and subdividing by version is just needless complexity (especially when we stick new tests in the most recent directory regardless whether they're relevant to that version's changes).

Comment 16

7 years ago
Comment on attachment 424883 [details] [diff] [review]
Updated

Code changes look good to me; the tests need to go in the right place.
Attachment #424883 - Flags: review?(jim) → review-
Created attachment 425129 [details] [diff] [review]
.
Attachment #424883 - Attachment is obsolete: true
Attachment #425129 - Flags: review?(jim)

Comment 18

7 years ago
Comment on attachment 425129 [details] [diff] [review]
.

Looks good, but (mea culpa) you'll need to fix up the jstest.list changes.  There is now a js/src/tests/ecma_5/extensions directory, so the patches to jstests.list and shell.js will need to be changed.  And doesn't js/src/tests/ecma_5/Expressions need to be added to js/src/tests/ecma_5/jstests.list?
Attachment #425129 - Flags: review?(jim) → review+

Comment 19

7 years ago
http://hg.mozilla.org/mozilla-central/rev/63efa8786575
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED

Comment 20

7 years ago
Is this something worth mentioning at https://developer.mozilla.org/en/Upcoming_Firefox_features_for_developers? I'm not experienced enough with JavaScript to tell, if it should be, myself.
Yes, probably, should have thought to add the keyword.
Keywords: dev-doc-needed
Appropriate notes have been added:

https://developer.mozilla.org/en/JavaScript/Reference/Operators/Special_Operators/set_Operator
https://developer.mozilla.org/en/JavaScript/Reference/Operators/Special_Operators/get_Operator
Keywords: dev-doc-needed → dev-doc-complete
Oops, wrong bug. This hasn't been done yet, I think.
Keywords: dev-doc-complete → dev-doc-needed
Actually, the same note that fixes the other bug fixes this one too, so this really is doc-complete.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.