8378740: Suppressed warnings reported when implicit compilation is combined with annotation processing#30110
8378740: Suppressed warnings reported when implicit compilation is combined with annotation processing#30110lahodaj wants to merge 5 commits intoopenjdk:masterfrom
Conversation
…mbined with annotation processing
|
👋 Welcome back jlahoda! A progress list of the required criteria for merging this PR into |
|
@lahodaj 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 199 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
Webrevs
|
david-beaumont
left a comment
There was a problem hiding this comment.
Looks good. A few optional suggestions.
| /* | ||
| * @test | ||
| * @bug 8378740 | ||
| * @summary Verify warnings are properly suppress in the combination of |
There was a problem hiding this comment.
nit: "suppress in" --> "suppressed for" ?
|
|
||
| public class APImplicitClassesWarnings { | ||
|
|
||
| ToolBox tb = new ToolBox(); |
There was a problem hiding this comment.
nit: final (and maybe private)
I know it's not super important but as a reader of code, it's nice to not have lingering questions about "why or when could this change". A final field can just be read once and completely understood.
| public void testCorrectSuppress() throws Exception { | ||
| Path src = base.resolve("src"); | ||
| Path classes = base.resolve("classes"); | ||
| tb.writeJavaFiles(src, |
There was a problem hiding this comment.
Maybe a comment saying that the only difference here is the one suppression line.
It saves me having to do a line-by-line scan to know what changed.
Update copyright year.
|
|
|
/integrate |
|
Going to push as commit 39a2566.
Your commit was automatically rebased without conflicts. |
As part of @david-beaumont's work on openjdk/valhalla#2180, it turned out warnings are sometimes reported to a wrong file when annotation processors are present, and, as a consequence, these warnings are not properly suppressed by
@SuppressWarnings.The reason is simple: the
log.useSourceis misplaced, and the source in theLogmay not be correctly reset. The source is correctly reset iftree == null, which is the normal case without annotation processing. But with annotation processing combined with implicit compilation,tree != null, and the source is never reset to the original value. And subsequent diagnostic then may be assigned to a wrong file.This PR is moving setting the source to the correct place.
Note that for
testCorrectSource, there are multiple warnings printed. This is an pre-existing problem that is sadly harder to fix albeit hopefully with a smaller severity. I think it would be better tackled separately. I've filled https://bugs.openjdk.org/browse/JDK-8378950 for it.Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/30110/head:pull/30110$ git checkout pull/30110Update a local copy of the PR:
$ git checkout pull/30110$ git pull https://git.openjdk.org/jdk.git pull/30110/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 30110View PR using the GUI difftool:
$ git pr show -t 30110Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/30110.diff
Using Webrev
Link to Webrev Comment