Last Comment Bug 520696 - Implement support for |{ get "string literal"() { /* ... */ }, get 5.4() { /* ... */ }, 6.72: 3 }|
: Implement support for |{ get "string literal"() { /* ... */ }, get 5.4() { /*...
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: mozilla1.9.3a1
Assigned To: Jeff Walden [:Waldo] (remove +bmo to email)
:
Mentors:
Depends on:
Blocks: es5 517580
  Show dependency treegraph
 
Reported: 2009-10-05 18:19 PDT by Jeff Walden [:Waldo] (remove +bmo to email)
Modified: 2011-02-03 12:00 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (8.72 KB, patch)
2009-12-15 09:59 PST, Jeff Walden [:Waldo] (remove +bmo to email)
no flags Details | Diff | Review
Updated (8.31 KB, patch)
2010-02-02 15:54 PST, Jeff Walden [:Waldo] (remove +bmo to email)
jimb: review-
Details | Diff | Review
. (8.37 KB, patch)
2010-02-03 18:11 PST, Jeff Walden [:Waldo] (remove +bmo to email)
jimb: review+
Details | Diff | Review

Description Jeff Walden [:Waldo] (remove +bmo to email) 2009-10-05 18:19:17 PDT
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.
Comment 1 Jeff Walden [:Waldo] (remove +bmo to email) 2009-10-05 18:25:21 PDT
Note that NumericLiteral is any number literal, possibly fractional, possibly exponential (even with positive or negative exponent), not just DecimalIntegerLiteral.
Comment 2 Jeff Walden [:Waldo] (remove +bmo to email) 2009-10-07 13:50:01 PDT
...which means we also have to handle { 3.141592654: "π" }.
Comment 3 Jeff Walden [:Waldo] (remove +bmo to email) 2009-12-15 09:59:52 PST
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.
Comment 4 Jim Blandy :jimb 2010-02-02 14:42:55 PST
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 Jim Blandy :jimb 2010-02-02 14:51:31 PST
(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 Jim Blandy :jimb 2010-02-02 14:57:43 PST
Shouldn't those tests go in js/src/tests/ecma_5/Expressions/11.1.5.js?  We're just implementing what the standard specifies.
Comment 7 Jeff Walden [:Waldo] (remove +bmo to email) 2010-02-02 15:33:36 PST
(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.
Comment 8 Jeff Walden [:Waldo] (remove +bmo to email) 2010-02-02 15:36:44 PST
(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.)
Comment 9 Brendan Eich [:brendan] 2010-02-02 15:45:03 PST
What's a patch-cest? If it's what it sounds like, the taboo against it stands :-/.

/be
Comment 10 Jeff Walden [:Waldo] (remove +bmo to email) 2010-02-02 15:54:07 PST
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.
Comment 11 Brendan Eich [:brendan] 2010-02-02 16:05:39 PST
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 Jim Blandy :jimb 2010-02-02 16:13:23 PST
(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 Jim Blandy :jimb 2010-02-02 16:14:51 PST
[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 Jim Blandy :jimb 2010-02-02 16:15:32 PST
(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.
Comment 15 Jeff Walden [:Waldo] (remove +bmo to email) 2010-02-02 16:23:49 PST
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 Jim Blandy :jimb 2010-02-02 16:49:22 PST
Comment on attachment 424883 [details] [diff] [review]
Updated

Code changes look good to me; the tests need to go in the right place.
Comment 17 Jeff Walden [:Waldo] (remove +bmo to email) 2010-02-03 18:11:26 PST
Created attachment 425129 [details] [diff] [review]
.
Comment 18 Jim Blandy :jimb 2010-04-02 17:03:47 PDT
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?
Comment 20 d 2010-04-15 10:12:12 PDT
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.
Comment 21 Jeff Walden [:Waldo] (remove +bmo to email) 2010-04-15 10:27:33 PDT
Yes, probably, should have thought to add the keyword.
Comment 23 Eric Shepherd [:sheppy] 2011-02-03 11:58:47 PST
Oops, wrong bug. This hasn't been done yet, I think.
Comment 24 Eric Shepherd [:sheppy] 2011-02-03 12:00:17 PST
Actually, the same note that fixes the other bug fixes this one too, so this really is doc-complete.

Note You need to log in before you can comment on or make changes to this bug.