Last Comment Bug 536472 - ES5: { get x(v) { } } and { set x(v, v2) { } } should be syntax errors
: ES5: { get x(v) { } } and { set x(v, v2) { } } should be syntax errors
Status: RESOLVED FIXED
fixed-in-tracemonkey
: dev-doc-complete
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Jeff Walden [:Waldo] (remove +bmo to email)
:
Mentors:
: 554616 (view as bug list)
Depends on: 581744
Blocks: es5
  Show dependency treegraph
 
Reported: 2009-12-22 15:13 PST by Jeff Walden [:Waldo] (remove +bmo to email)
Modified: 2011-02-03 11:58 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
1: Move variable definitions in Parser::functionDef as close to uses as possible (4.08 KB, patch)
2010-08-02 13:13 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
cdleary: review+
Details | Diff | Review
2: Don't pointlessly set JSPROP_{G,S}ETTER in fun->flags (1.56 KB, patch)
2010-08-02 13:15 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
cdleary: review+
Details | Diff | Review
3: Move function name parsing to callers (3.64 KB, patch)
2010-08-02 13:16 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
cdleary: review+
Details | Diff | Review
4: Move arguments parsing into a separate function (16.65 KB, patch)
2010-08-02 13:19 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
cdleary: review+
Details | Diff | Review
5: Actually check the number of arguments to parse (18.69 KB, patch)
2010-08-02 14:02 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
cdleary: review+
Details | Diff | Review

Description Jeff Walden [:Waldo] (remove +bmo to email) 2009-12-22 15:13:16 PST

    
Comment 1 Jeff Walden [:Waldo] (remove +bmo to email) 2010-04-21 17:00:57 PDT
*** Bug 554616 has been marked as a duplicate of this bug. ***
Comment 2 Jeff Walden [:Waldo] (remove +bmo to email) 2010-08-02 12:40:48 PDT
Ugh, function parsing is a disaster right now, no way this should have been [good first bug].  Patches coming up to do this...
Comment 3 Jeff Walden [:Waldo] (remove +bmo to email) 2010-08-02 13:13:51 PDT
Created attachment 462146 [details] [diff] [review]
1: Move variable definitions in Parser::functionDef as close to uses as possible

It's hard to understand possible syntactic transformations when you have to worry about effects on half a dozen apparently-non-extraneous variables.
Comment 4 Jeff Walden [:Waldo] (remove +bmo to email) 2010-08-02 13:15:01 PDT
Created attachment 462148 [details] [diff] [review]
2: Don't pointlessly set JSPROP_{G,S}ETTER in fun->flags

...following up on bug 581744.
Comment 5 Jeff Walden [:Waldo] (remove +bmo to email) 2010-08-02 13:16:30 PDT
Created attachment 462149 [details] [diff] [review]
3: Move function name parsing to callers

Making one Swiss army knife function parse names doesn't make sense when different callers have different naming requirements -- better for them to compute the name themselves and pass it in.
Comment 6 Jeff Walden [:Waldo] (remove +bmo to email) 2010-08-02 13:19:19 PDT
Created attachment 462150 [details] [diff] [review]
4: Move arguments parsing into a separate function

Parser::functionDef is monstrously huge, 400+ lines before any of these patches.  Splitting it up a bit (here by moving argument parsing out of the main function), done with even a slight amount of discernment, can only help readability.
Comment 7 Jeff Walden [:Waldo] (remove +bmo to email) 2010-08-02 14:02:16 PDT
Created attachment 462180 [details] [diff] [review]
5: Actually check the number of arguments to parse

I was hoping to make more cleanups here, and to eventually move argument parsing into the callers, but the number of enclosing-scope variables required to parse arguments is pretty large, and boxing up a struct of them is just ugly.  There must be a better way to do this that doesn't involve everything intermingled like here, but I'm not sure it's an incremental step away, as the previous patches have been.  I lack the energy and time to do anything more than an incremental step, so this is where the cleanup stops and the special-casing begins -- on to other bugs with this done, now.

I haven't run through tests other than js tests to see whether any others need updates for this change -- some probably do, but those changes aren't the interesting part of this patch, from the JS engine point of view.
Comment 8 Brendan Eich [:brendan] 2010-08-02 17:21:46 PDT
Nice work, Waldo!

Could maybe use the split-out functionArguments parser from jsfun.cpp:Function, but then we'd accept destructuring formals. An allowed extension? Language lawyer needed, ES5 Chapter 16, also Chapter 15 intro, has revisions to tighten rules.

/be
Comment 9 Chris Leary [:cdleary] (not checking bugmail) 2010-08-17 11:42:30 PDT
Comment on attachment 462149 [details] [diff] [review]
3: Move function name parsing to callers

Yup, pushing name parsing into the callers makes more sense.
Comment 10 Chris Leary [:cdleary] (not checking bugmail) 2010-08-17 12:09:43 PDT
Comment on attachment 462180 [details] [diff] [review]
5: Actually check the number of arguments to parse

Please add tests for functions with duplicate formals. Other than that, looks great.

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