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

fix(java): Stop using Streams to render params #459

Merged
merged 1 commit into from
Apr 16, 2019

Conversation

RomainMuller
Copy link
Contributor

@RomainMuller RomainMuller commented Apr 16, 2019

The Java cod generated by jsii-pacmak used to use Stream
concatenations to render parameters into an Object array when passing
to the low-level jsii engine. In the majority of cases this worked
fine, however in cases where the last argument was an enum, this
caused javac type inferrence to impose bounds on the Stream's item
type that could not be met by non-enum types.

Additionally, as this code demonstrates, the Stream-based code was
unnecessarily complicated as it is possible to initialize an array
literal in place, by using less code, which also reads simpler, and
incurs less memory churn. It's a win all over the board 🎉

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

The Java cod egenerated by `jsii-pacmak` used to use `Stream`
concatenations to render parameters into an `Object` array when passing
to the low-level `jsii` engine. In the majority of cases this worked
fine, however in casesw where the last argument was an `enum`, this
caused `javac` type inferrence to impose bounds on the `Stream`'s item
type that could not be met by non-`enum` types.

Additionally, as this code demonstrates, the `Stream`-based code was
unnecessarily complicated as it is possible to initialize an array
literal in place, by using less code, which also reads simpler, and
incurs less memory churn. It's a win all over the board 🎉
@RomainMuller RomainMuller requested a review from a team as a code owner April 16, 2019 07:44
Copy link
Contributor

@eladb eladb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes please!

@RomainMuller RomainMuller merged commit a5e8a93 into master Apr 16, 2019
@RomainMuller RomainMuller deleted the rmuller/fix-java-args branch April 16, 2019 08:13
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.

2 participants