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

JDK-19 adds String::valueOf calls to string concatenation #151

Closed
skyrising opened this issue May 25, 2022 · 6 comments · Fixed by #153
Closed

JDK-19 adds String::valueOf calls to string concatenation #151

skyrising opened this issue May 25, 2022 · 6 comments · Fixed by #153
Labels
enhancement New feature or request good first issue Good for newcomers Priority: Low Low priority Subsystem: Statement Structure Anything concerning how statements are structured in a method

Comments

@skyrising
Copy link
Contributor

With openjdk/jdk@cfee451 extra String::valueOf calls are inserted for objects passed to makeConcatWithConstants. Quiltflower should remove these for better readability.

@skyrising skyrising added enhancement New feature or request Subsystem: Statement Structure Anything concerning how statements are structured in a method Priority: Low Low priority labels May 25, 2022
@jaskarth jaskarth added the good first issue Good for newcomers label May 26, 2022
@thewindsofwinter
Copy link
Contributor

Hi, I can probably write this up real quick. Let me know where I should look!

@jaskarth
Copy link
Member

Hey, thanks! I'd recommend that you look here, and just before the return insert a loop that goes over the parameters array and call, and runs the existing removeStringValueOf method over each exprent, to remove the valueOf call.

@thewindsofwinter
Copy link
Contributor

Understood! Shouldn't be too difficult a fix, definitely will get it done within a few hours once I get a bit of free time. Question: how should we run tests before committing? Is there code style that I'll need to follow/recommended linting? Should I squash commits (probably unnecessary, my guess is I'll get it in one). Let me know, thanks!

@thewindsofwinter
Copy link
Contributor

Just added a loop in extractParameters, hopefully what you wanted (since the only other place those parameters come up via a call to extractParameters). Let me know if that's what you want! Also, out of curiosity how does this integrate into the overall code? It's a little hard for a newcomer to understand 😅

@jaskarth
Copy link
Member

jaskarth commented May 26, 2022

Hey! Generally the best way to run tests is to use gradle's test feature or to create a junit configuration in IntelliJ IDEA if you use that. I forgot to attach my class file, but here: TestStringConcatJ19.zip (Added as a zip because github doesn't like class files) You should add this to the testData/classes/custom directory, and then add the name of the class to the SingleClassesTest class. Now you can test out your changes when you run the test suite!

Also I totally get the code being a bit hard to get a grasp of at first, it's a lot of things to take in at once- But to summarize, this is the main method processing class. If you take a look at that class, you'll see that there are a lot of transformations that are run on the code, for example finding switch expressions, creating for and foreach loops, and more. The highlighted bit of code there handles resugaring string concat for Java 9+, as string concat in that version is done with invokedynamic instead of chained StringBuilder calls as it once was. The code that you're changing handles each invokedynamic instance seperately, so you are working with a single concatenation at a time in your change. Hope that cleared things up, if you have any more questions please feel free to ask! If it's more convenient, you can also find us on Discord, in #decompilers-general :)

jaskarth added a commit that referenced this issue May 26, 2022
…valueof

Resolves #151: Remove Unnecessary String.valueOfs()
@jaskarth
Copy link
Member

Fixed by #153

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers Priority: Low Low priority Subsystem: Statement Structure Anything concerning how statements are structured in a method
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants