-
Notifications
You must be signed in to change notification settings - Fork 905
Support for rest parameters in destructuring #2207
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
9c9409f to
58b4fb0
Compare
58b4fb0 to
e7d8a82
Compare
|
This is great progress -- when you all get back to work, let me know if someone has time for a code review or if they're already OK with this. |
e7d8a82 to
d3a7ab0
Compare
aardvark179
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this looks good, but I there's a big TODO list on the ScriptRuntime change, and byte code / class compilation seems complex in some ways we might avoid. I think it's worth seeing if we improve those areas.
| out.println(tname + " \"" + str + '"'); | ||
| break; | ||
| } | ||
| case Icode_OBJECT_REST: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I like the way this byte code is structured, it's the first time we've had a code whose length depend on its data in this way, but I understand how you got there. How big a change would it be to restructure the two complex literal types in InterpreterData (regexes and template literals) as a single Object[] and put your data structure in that?
Edit: Ah, I see the class compiled version is doing the same sort of thing. Maybe this is the point to start moving this sort of complex data onto the JSDesccriptor and see if that helps tackle the big TODO in the ScriptRuntime change.
| * @param excludeKeys array of property names to exclude | ||
| * @return a new object with all properties except the excluded ones | ||
| */ | ||
| // TODO - simplify: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a pretty big TODO list. 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, addressed the problems in the TODO list. Will work on restructuring bytecode next.
Thanks for the review. I'll work on addressing the items in the TODO list. I kinda left it for another day as this PR was already getting big. But, happy to address them. :) |
Adds support for rest parameters in destructuring patterns (Object & Array).
The remaining failures in test262 from enabling "object-rest" are due to pending items with: