Skip to content
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

Compile splats in arrays and function calls to ES2015 splats #4353

Merged
merged 1 commit into from
Nov 6, 2016
Merged

Compile splats in arrays and function calls to ES2015 splats #4353

merged 1 commit into from
Nov 6, 2016

Conversation

connec
Copy link
Collaborator

@connec connec commented Nov 5, 2016

Rather than compiling splats to arrays built using Array#concat, splats are now compiled directly to ES2015 splats, e.g.

f foo, arguments..., bar
[ foo, arguments..., bar ]

Which used to be compiled to:

f.apply(null, [foo].concat(slice.call(arguments), [bar]));
[foo].concat(slice.call(arguments), [bar]);

Is now compiled to:

f(foo, ...arguments, bar);
[ foo, ...arguments, bar ];

This was really easy to implement, and mostly involved deleting code. No tests were broken, and I can't think of any extra tests to add.

@GeoffreyBooth
Copy link
Collaborator

This . . . looks awesome. Could it really be this simple? I guess this was low-hanging fruit.

The changes (simplifications) to the compiled code kind of speaks for itself. I can confirm that the tests (including harmony ones) still pass. @lydell, anyone else, any other notes?

@GeoffreyBooth
Copy link
Collaborator

Do we need any new tests to cover anything related to outputting ES2015 splat syntax?

Also I’d love to make the side that the ... go on optional, so that both args... and ...args work, but that should be a separate PR.

@lydell
Copy link
Collaborator

lydell commented Nov 6, 2016

Implementation-wise I can’t think of anything else to do. Good job!

Testing-wise, I guess we could add a test involving generators.

$ node -p 'a = function*(){yield 1; yield 2; yield 3}; [...a()]'
[ 1, 2, 3 ]

$ coffee -e 'a = (-> yield 1; yield 2; yield 3); console.log [a()...]'
[]

With this PR, I’d expect both of the above commands to output [ 1, 2, 3 ].

Also I’d love to make the side that the ... go on optional, so that both args... and ...args work, but that should be a separate PR.

Me too! I’d even like to have that in master. I also agree that it should be a separate PR.

Rather than compiling splats to arrays built using `Array#concat`, splats
are now compiled directly to ES2015 splats, e.g.

    f foo, arguments..., bar

    [ foo, arguments..., bar ]

Which used to be compiled to:

    f.apply(null, [foo].concat(slice.call(arguments), [bar]));

    [foo].concat(slice.call(arguments), [bar]);

Is now compiled to:

    f(foo, ...arguments, bar);

    [ foo, ...arguments, bar ];
@connec
Copy link
Collaborator Author

connec commented Nov 6, 2016

Also I’d love to make the side that the ... go on optional, so that both args... and ...args work, but that should be a separate PR.

Me too! I’d even like to have that in master. I also agree that it should be a separate PR.

Aww :( I feel like we should have 'one way' of doing splats. I'd lament switching to ES2015's prefix notation, but that'd be better than doing both imo.

At any rate, I've added that test and it passes fine 😄

@lydell
Copy link
Collaborator

lydell commented Nov 6, 2016

I guess we’re good to go then!

@GeoffreyBooth GeoffreyBooth merged commit 663595b into jashkenas:2 Nov 6, 2016
@GeoffreyBooth
Copy link
Collaborator

Thanks @connec, great work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants