-
Notifications
You must be signed in to change notification settings - Fork 6.1k
8276926: Use String.valueOf() when initializing File.separator and File.pathSeparator #6326
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
Conversation
|
👋 Welcome back stsypanov! A progress list of the required criteria for merging this PR into |
|
@stsypanov The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
|
it should compile into invokedynamic makeConcatWithConstants on later JDKs, but valueOf(char) is most likely going to have smaller first-invocation overhead still. |
As far as I understand |
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.
FWIW the specific pattern of "" + foo could probably profitably translate into String.valueOf(foo) regardless of StringConcatFactory availability. (And translating String concat to use SCF in class initializers is likely a pessimization most of the time.) I'm no expert on javac but I suspect implementing (and worse: specifying) minor improvements to the translation might be more effort than it's worth, though, so point fixes like these seem fine to me.
|
|
@stsypanov This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 3 new commits pushed to the
Please see this link for an up-to-date comparison between the source branch of this pull request and the As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@cl4es, @JimLaskey) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
|
/integrate |
|
@stsypanov |
|
/sponsor |
|
Going to push as commit 0f23c6a.
Your commit was automatically rebased without conflicts. |
|
@cl4es @stsypanov Pushed as commit 0f23c6a. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
|
It's unlikely that javac would do this kind of optimization (javac tends to produce code verbatim), but it might be worth filing a RFE. |
|
I've filed https://bugs.openjdk.java.net/browse/JDK-8276951 This pattern is quite common - both in the OpenJDK and elsewhere. |
Looking into
File.pathSeparatorI've found out that currently we initialize it asWhich after compilation turns into
This can be simplified to
Which is in turn complied into more compact
The changed code is likely to slightly improve start-up time as it allocates less and does no bound checks.
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/6326/head:pull/6326$ git checkout pull/6326Update a local copy of the PR:
$ git checkout pull/6326$ git pull https://git.openjdk.java.net/jdk pull/6326/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 6326View PR using the GUI difftool:
$ git pr show -t 6326Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/6326.diff