The default bug view has changed. See this FAQ.

Reflect.parse needs to be updated to support defaults and rest parameters

RESOLVED FIXED in mozilla16

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Benjamin, Assigned: Benjamin)

Tracking

unspecified
mozilla16
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
Created attachment 629676 [details] [diff] [review]
support rest and defaults
Attachment #629676 - Flags: review?(dherman)
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(...rest) { }",
>+           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 https://developer.mozilla.org/en/SpiderMonkey/Parser_API 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.

Thanks,
Dave
Attachment #629676 - Flags: review?(dherman) → review+
(Assignee)

Comment 3

5 years ago
I think your suggested API is better.
(Assignee)

Comment 4

5 years ago
Created attachment 630873 [details] [diff] [review]
store rest as an identifier separate from params
Attachment #629676 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
(Assignee)

Comment 5

5 years ago
The wiki has been updated.
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/c2ce10a018d8
Flags: in-testsuite+
Target Milestone: --- → mozilla16
https://hg.mozilla.org/mozilla-central/rev/c2ce10a018d8
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.