Closed Bug 520696 Opened 11 years ago Closed 11 years ago

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

Categories

(Core :: JavaScript Engine, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.3a1

People

(Reporter: Waldo, Assigned: Waldo)

References

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 2 obsolete files)

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 }|
Attached patch Patch (obsolete) — Splinter Review
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)
Curious --- why the initialization of funAtom in FunctionDef?  That's definitely set by the first 'if'; was some compiler too dumb to see that?
(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.
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
Attached patch Updated (obsolete) — Splinter Review
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
(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
[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.
(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 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-
Attached patch .Splinter Review
Attachment #424883 - Attachment is obsolete: true
Attachment #425129 - Flags: review?(jim)
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+
http://hg.mozilla.org/mozilla-central/rev/63efa8786575
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
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
Oops, wrong bug. This hasn't been done yet, I think.
Actually, the same note that fixes the other bug fixes this one too, so this really is doc-complete.
You need to log in before you can comment on or make changes to this bug.