Last Comment Bug 760304 - Reflect.parse needs to be updated to support defaults and rest parameters
: Reflect.parse needs to be updated to support defaults and rest parameters
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
-- normal (vote)
: mozilla16
Assigned To: :Benjamin Peterson
: Jason Orendorff [:jorendorff]
Depends on: 759498
  Show dependency treegraph
Reported: 2012-05-31 15:41 PDT by :Benjamin Peterson
Modified: 2012-06-09 19:49 PDT (History)
4 users (show)
dzbarsky: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

support rest and defaults (13.88 KB, patch)
2012-06-03 18:08 PDT, :Benjamin Peterson
dherman: review+
Details | Diff | Splinter Review
store rest as an identifier separate from params (14.08 KB, patch)
2012-06-06 23:43 PDT, :Benjamin Peterson
no flags Details | Diff | Splinter Review

Description User image :Benjamin Peterson 2012-05-31 15:41:10 PDT
The reflect AST needs to have additions for rest parameters and defaults. I need bug 759498 first, since it alters the parse tree for defaults.
Comment 1 User image :Benjamin Peterson 2012-06-03 18:08:35 PDT
Created attachment 629676 [details] [diff] [review]
support rest and defaults
Comment 2 User image Dave Herman [:dherman] 2012-06-06 15:28:07 PDT
Comment on attachment 629676 [details] [diff] [review]
support rest and defaults

>     return newNode(type, pos,
>                    "id", id,
>                    "params", array,
>+                   "defaults", defarray,
>                    "body", body,
>+                   "rest", BooleanValue(hasRest),

The rest-argument has a name -- are you including the name in the params list and this just indicates whether the last name is a rest-arg? Would it be a nicer API to have the params array only have the non-rest params, and the .rest field to be either a string or null?

>                    "generator", BooleanValue(isGenerator),
>                    "expression", BooleanValue(isExpression),
>                    dst);
> }

>diff --git a/js/src/tests/js1_8_5/extensions/reflect-parse.js b/js/src/tests/js1_8_5/extensions/reflect-parse.js
>--- a/js/src/tests/js1_8_5/extensions/reflect-parse.js
>+++ b/js/src/tests/js1_8_5/extensions/reflect-parse.js
>@@ -20,24 +20,28 @@ var _ = Pattern.ANY;


>+assertDecl("function foo( { }",
>+           funDecl(ident("foo"), [ident("rest")], blockStmt([]), [], true));

Yeah, so I was thinking it might make more sense for the API to return

    funDecl(ident("foo"), [], blockStmt([]), [], "rest")

But I'm not adamant about this. It just seems a little less error-prone somehow. What do you think?

Please update the docs at if you haven't already.

Overall looks good. r=me; it's up to you if you want to make that API change I suggested, or we can discuss in the bug.

Comment 3 User image :Benjamin Peterson 2012-06-06 23:39:27 PDT
I think your suggested API is better.
Comment 4 User image :Benjamin Peterson 2012-06-06 23:43:13 PDT
Created attachment 630873 [details] [diff] [review]
store rest as an identifier separate from params
Comment 5 User image :Benjamin Peterson 2012-06-06 23:43:52 PDT
The wiki has been updated.
Comment 6 User image David Zbarsky (:dzbarsky) 2012-06-09 15:22:29 PDT
Comment 7 User image Ryan VanderMeulen [:RyanVM] 2012-06-09 19:49:22 PDT

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