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.