8346294: Invalid lint category specified in compiler.properties #22769

archiecobbs · 2024-12-16T16:54:46Z

Please review this fix for an incorrect lint: tag in compiler.properties, plus an adjustment to the build process to automatically detect and fail the build in case of any similar typos in the future.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8346294: Invalid lint category specified in compiler.properties (Bug - P4)

Reviewers

Maurizio Cimadamore (@mcimadamore - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/22769/head:pull/22769
$ git checkout pull/22769

Update a local copy of the PR:
$ git checkout pull/22769
$ git pull https://git.openjdk.org/jdk.git pull/22769/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 22769

View PR using the GUI difftool:
$ git pr show -t 22769

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/22769.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2024-12-16T16:55:36Z

👋 Welcome back acobbs! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2024-12-16T16:55:39Z

@archiecobbs 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:

8346294: Invalid lint category specified in compiler.properties

Reviewed-by: mcimadamore

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 14 new commits pushed to the master branch:

18d1d61: 8346046: Enable copyright header format check
a7631cc: 8346235: RISC-V: Optimize bitwise AND with mask values
929d4a5: 8346231: RISC-V: Fix incorrect assertion in SharedRuntime::generate_handler_blob
3030230: 8346278: Clean up some flag handing in flags-cflags.m4
fd0207d: 8345327: JDK 24 RDP1 L10n resource files update
31c3b19: 8346174: UMAX/UMIN are missing from XXXVector::reductionOperations
c75b1d4: 8346082: Output JVMTI agent information in hserr files
51662c2: 8346232: Remove leftovers of the jar --normalize feature
d335941: 8345678: compute_modifiers should not be in create_mirror
57adf64: 8346261: Cleanup in JDP tests
... and 4 more: https://git.openjdk.org/jdk/compare/d737978924f08f1aae280867e5106f2cfd0e9d31...master

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 master branch, type /integrate in a new comment.

openjdk · 2024-12-16T16:56:18Z

@archiecobbs The following labels will be automatically applied to this pull request:

build
compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

mlbridge · 2024-12-16T16:58:58Z

Webrevs

01: Full - Incremental (3f792a8b)
00: Full (1ce466fa)

mcimadamore · 2024-12-16T18:35:44Z

src/jdk.compiler/share/classes/com/sun/tools/javac/code/Lint.java

+         * @return corresponding {@link LintCategory}
+         * @throws IllegalArgumentException if no such lint category exists
+         */
+        public static LintCategory forOption(String option) {


Maybe we should call this "getOrThrow" ?

Yeah, or perhaps the most modern & proper thing to do would be to have LintCategory.get() return Optional<LintCategory>.

This method is only used in a couple of places so that refactoring seems safe.

Updated in 3f792a8.

mcimadamore · 2024-12-16T18:36:06Z

Thanks for catching this

mcimadamore · 2024-12-16T18:37:41Z

test/langtools/tools/javac/lint/LintWarningCategoryTest.java

+
+import com.sun.tools.javac.resources.CompilerProperties.LintWarnings;
+
+public class LintWarningCategoryTest {


Do we need this specific test? Or will either the build, or other tests fail in case there's a lint category/name mismatch?

We don't strictly need this test, because build will fail now if any lint: label is invalid, so the test is redundant.

But I also wasn't sure if it was OK to not have any test at all.

Another option would be to split this change two separate bugs/PRs, but that seems like overkill.

(flips through OpenJDK developer guide...) Maybe the best answer would to remove the test and add the noreg-build label, as this can be considered a build fix.

Let's try that route... updated in 3f792a8.

yeah, it's ok not to have a test, but you need some keyword, which you added.

mcimadamore · 2024-12-16T19:26:56Z

src/jdk.compiler/share/classes/com/sun/tools/javac/code/Lint.java

-                LintCategory lc = LintCategory.get((String) (value.value));
-                if (lc != null)
-                    suppress(lc);
+                LintCategory.get((String)value.value)


I think use of Optional makes sense here too, so bonus point for the extra consolidation!

archiecobbs · 2024-12-16T20:45:41Z

Thanks for the quick review!

archiecobbs · 2024-12-17T03:17:09Z

/integrate

openjdk · 2024-12-17T03:18:27Z

Going to push as commit 87804f2.
Since your change was applied there have been 14 commits pushed to the master branch:

18d1d61: 8346046: Enable copyright header format check
a7631cc: 8346235: RISC-V: Optimize bitwise AND with mask values
929d4a5: 8346231: RISC-V: Fix incorrect assertion in SharedRuntime::generate_handler_blob
3030230: 8346278: Clean up some flag handing in flags-cflags.m4
fd0207d: 8345327: JDK 24 RDP1 L10n resource files update
31c3b19: 8346174: UMAX/UMIN are missing from XXXVector::reductionOperations
c75b1d4: 8346082: Output JVMTI agent information in hserr files
51662c2: 8346232: Remove leftovers of the jar --normalize feature
d335941: 8345678: compute_modifiers should not be in create_mirror
57adf64: 8346261: Cleanup in JDP tests
... and 4 more: https://git.openjdk.org/jdk/compare/d737978924f08f1aae280867e5106f2cfd0e9d31...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-12-17T03:18:34Z

@archiecobbs Pushed as commit 87804f2.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Fix invalid "lint" tag and update build to automatically detect.

1ce466f

openjdk bot added the rfr Pull request is ready for review label Dec 16, 2024

openjdk bot added build build-dev@openjdk.org compiler compiler-dev@openjdk.org labels Dec 16, 2024

mcimadamore reviewed Dec 16, 2024

View reviewed changes

Cleanups & refactoring based on review suggestions.

3f792a8

mcimadamore reviewed Dec 16, 2024

View reviewed changes

mcimadamore approved these changes Dec 16, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Dec 16, 2024

openjdk bot added the integrated Pull request has been integrated label Dec 17, 2024

openjdk bot closed this Dec 17, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Dec 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8346294: Invalid lint category specified in compiler.properties #22769

8346294: Invalid lint category specified in compiler.properties #22769

archiecobbs commented Dec 16, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Dec 16, 2024

openjdk bot commented Dec 16, 2024 •

edited

Loading

openjdk bot commented Dec 16, 2024

mlbridge bot commented Dec 16, 2024 •

edited

Loading

mcimadamore Dec 16, 2024

archiecobbs Dec 16, 2024

mcimadamore commented Dec 16, 2024

mcimadamore Dec 16, 2024

archiecobbs Dec 16, 2024

mcimadamore Dec 16, 2024

mcimadamore Dec 16, 2024

archiecobbs commented Dec 16, 2024

archiecobbs commented Dec 17, 2024

openjdk bot commented Dec 17, 2024

openjdk bot commented Dec 17, 2024


		import com.sun.tools.javac.resources.CompilerProperties.LintWarnings;

		public class LintWarningCategoryTest {

8346294: Invalid lint category specified in compiler.properties #22769

8346294: Invalid lint category specified in compiler.properties #22769

Conversation

archiecobbs commented Dec 16, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Dec 16, 2024

openjdk bot commented Dec 16, 2024 • edited Loading

openjdk bot commented Dec 16, 2024

mlbridge bot commented Dec 16, 2024 • edited Loading

Webrevs

mcimadamore Dec 16, 2024

Choose a reason for hiding this comment

archiecobbs Dec 16, 2024

Choose a reason for hiding this comment

mcimadamore commented Dec 16, 2024

mcimadamore Dec 16, 2024

Choose a reason for hiding this comment

archiecobbs Dec 16, 2024

Choose a reason for hiding this comment

mcimadamore Dec 16, 2024

Choose a reason for hiding this comment

mcimadamore Dec 16, 2024

Choose a reason for hiding this comment

archiecobbs commented Dec 16, 2024

archiecobbs commented Dec 17, 2024

openjdk bot commented Dec 17, 2024

openjdk bot commented Dec 17, 2024

archiecobbs commented Dec 16, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Dec 16, 2024 •

edited

Loading

mlbridge bot commented Dec 16, 2024 •

edited

Loading