-
Notifications
You must be signed in to change notification settings - Fork 746
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
Optimize VisitorState#getConstantExpression #4586
base: master
Are you sure you want to change the base?
Changes from all commits
c29a38e
dec3f87
624fa2d
07dfafa
4f61c1b
10b06e7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -181,6 +181,22 @@ | |
</jdkToolchain> | ||
</configuration> | ||
</execution> | ||
<execution> | ||
<id>java23</id> | ||
<goals> | ||
<goal>compile</goal> | ||
</goals> | ||
<configuration> | ||
<jdkToolchain> | ||
<version>23</version> | ||
</jdkToolchain> | ||
<compileSourceRoots> | ||
<compileSourceRoot>${basedir}/src/main/java23</compileSourceRoot> | ||
</compileSourceRoots> | ||
<!-- multiReleaseOutput requires setting release --> | ||
<outputDirectory>${project.build.outputDirectory}/META-INF/versions/23</outputDirectory> | ||
</configuration> | ||
</execution> | ||
<execution> | ||
<id>java24</id> | ||
<configuration> | ||
|
@@ -194,9 +210,36 @@ | |
<outputDirectory>${project.build.outputDirectory}/META-INF/versions/24</outputDirectory> | ||
</configuration> | ||
</execution> | ||
<execution> | ||
<!-- This final no-op compilation step resets the output directory associated with this | ||
module. This is important for Maven reactor builds that skip the `install` phase, | ||
because in those the compilation of a module `B` happens with reference to the output | ||
directory of a module `A` on which it depends, rather than `A`'s installed JAR. --> | ||
<id>reset-output-directory</id> | ||
<goals> | ||
<goal>compile</goal> | ||
</goals> | ||
<configuration> | ||
<compileSourceRoots> | ||
<compileSourceRoot>${basedir}/nonexistent-directory</compileSourceRoot> | ||
</compileSourceRoots> | ||
<outputDirectory>${project.build.outputDirectory}</outputDirectory> | ||
</configuration> | ||
</execution> | ||
</executions> | ||
|
||
</plugin> | ||
<plugin> | ||
<groupId>org.apache.maven.plugins</groupId> | ||
<artifactId>maven-jar-plugin</artifactId> | ||
<configuration> | ||
<archive> | ||
<manifestEntries> | ||
<Multi-Release>true</Multi-Release> | ||
</manifestEntries> | ||
</archive> | ||
</configuration> | ||
</plugin> | ||
Comment on lines
+232
to
+242
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also had to add this to make sure that the class file bundled under |
||
</plugins> | ||
</build> | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,46 @@ | ||
/* | ||
* Copyright 2024 The Error Prone Authors. | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
|
||
package com.google.errorprone; | ||
|
||
import com.sun.tools.javac.util.Convert; | ||
import javax.lang.model.util.Elements; | ||
|
||
/** | ||
* @see VisitorState#getConstantExpression(Object) | ||
*/ | ||
final class ConstantStringExpressions { | ||
private ConstantStringExpressions() {} | ||
Comment on lines
+22
to
+26
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Certainly open to approaches other than a special-purpose static utility class named There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems good for now, thanks. |
||
|
||
static String toConstantStringExpression(Object value, Elements elements) { | ||
if (!(value instanceof CharSequence)) { | ||
return elements.getConstantExpression(value); | ||
} | ||
|
||
// Don't escape single-quotes in string literals. | ||
CharSequence str = (CharSequence) value; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Dropped the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We're finally able to use Java 21 features here internally, the earlier comment is obsolete. |
||
StringBuilder sb = new StringBuilder("\""); | ||
for (int i = 0; i < str.length(); i++) { | ||
char c = str.charAt(i); | ||
if (c == '\'') { | ||
sb.append('\''); | ||
} else { | ||
sb.append(Convert.quote(c)); | ||
} | ||
} | ||
return sb.append('"').toString(); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,33 @@ | ||
/* | ||
* Copyright 2024 The Error Prone Authors. | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
|
||
package com.google.errorprone; | ||
|
||
import com.google.errorprone.VisitorState; | ||
import com.sun.tools.javac.util.Convert; | ||
import javax.lang.model.util.Elements; | ||
|
||
/** | ||
* @see VisitorState#getConstantExpression(Object) | ||
*/ | ||
final class ConstantStringExpressions { | ||
private ConstantStringExpressions() {} | ||
|
||
static String toConstantStringExpression(Object value, Elements elements) { | ||
return elements.getConstantExpression( | ||
value instanceof CharSequence charSequence ? charSequence.toString() : value); | ||
} | ||
} |
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.
A few things are of note here:
<goal>compile</goal>
this execution is skipped. This also means that thejava24
execution below is currently a no-op.<source>17</source>
and<target>17</target>
settings, though it could. (If so, we should do the same below.)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.
Huh, thanks for pointing that out. This is also used with an Bazel-based build that does the multi-release jar stuff differently, so maybe there are no uses (or test coverage) for the maven build and ErrorProneSignatureGenerator that require JDK 24 support.
The answer might be 'it depends'. For javac API changes, it's probably best to try to use an exact match, but that does require installing extra JDKs for the github actions setup. If the API changes once and is stable on newer versions we'd get away with a newer version, though.