8368497: [lworld] redo: New lint category initialization for code that would not be allowed in the prologue#1650
Conversation
…allowed in the prologue
|
👋 Welcome back vromero! A progress list of the required criteria for merging this PR into |
|
@vicente-romero-oracle 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 ➡️ To integrate this PR with the above commit message to the |
Webrevs
|
| * jdk.compiler/com.sun.tools.javac.file | ||
| * jdk.compiler/com.sun.tools.javac.main | ||
| * jdk.compiler/com.sun.tools.javac.tree | ||
| * jdk.compiler/com.sun.tools.javac.util |
There was a problem hiding this comment.
Can we put all these @modules into TEST.properties like this: https://github.com/openjdk/jdk/blob/db6320df980ebe7cf2a1c727970cc937ab549b97/test/jdk/jdk/classfile/TEST.properties#L2-L5
| if (p.toString().endsWith("java")) { | ||
| javaFile = p; | ||
| } else if (p.toString().endsWith("out")) { | ||
| goldenFile = p; |
There was a problem hiding this comment.
Can we just do baseDir.resolve(className + ".java"), and do a Files.exists check for the golden file, instead of having complicated stuf like this?
There was a problem hiding this comment.
yep I wanted to clean that before pushing but then I forgot, thanks
There was a problem hiding this comment.
Instead of patching individual microbenchmarks, we should just include initialization here:
valhalla/make/test/BuildMicrobenchmark.gmk
Lines 86 to 87 in 2bc50b3
There was a problem hiding this comment.
We should probably ignore initialization warnings for test libs at:
valhalla/make/test/BuildTestLib.gmk
Line 65 in 2bc50b3
There was a problem hiding this comment.
yes we could, but I prefer not to. I found bugs in the original implementation by analyzing case by case the reason for the warnings. If we remove it for all tests then we could be blinded to lwrking bugs
Co-authored-by: Chen Liang <liach@openjdk.org>
…fo.java Co-authored-by: Chen Liang <liach@openjdk.org>
|
/integrate |
|
@liach thanks for the comments |
|
Going to push as commit 1a33b86.
Your commit was automatically rebased without conflicts. |
|
@vicente-romero-oracle Pushed as commit 1a33b86. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
redoing JDK-8367698 as the original fix had to be delta applied due to some test failures
Progress
Issue
initializationfor code that would not be allowed in the prologue (Bug - P4)Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/valhalla.git pull/1650/head:pull/1650$ git checkout pull/1650Update a local copy of the PR:
$ git checkout pull/1650$ git pull https://git.openjdk.org/valhalla.git pull/1650/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 1650View PR using the GUI difftool:
$ git pr show -t 1650Using diff file
Download this PR as a diff file:
https://git.openjdk.org/valhalla/pull/1650.diff
Using Webrev
Link to Webrev Comment