On Mon, 16 Dec 2024 18:35:44 GMT, Maurizio Cimadamore <mcimadam...@openjdk.org>
wrote:
>> Archie Cobbs has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Cleanups & refactoring based on review suggestions.
>
> src/jdk.compiler/share/classes/com/sun/tools/javac/code/Lint.java line 383:
>
>> 381: * @throws IllegalArgumentException if no such lint category
>> exists
>> 382: */
>> 383: 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 3f792a8b650.
> test/langtools/tools/javac/lint/LintWarningCategoryTest.java line 36:
>
>> 34: import com.sun.tools.javac.resources.CompilerProperties.LintWarnings;
>> 35:
>> 36: 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 3f792a8b650.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/22769#discussion_r1887370472
PR Review Comment: https://git.openjdk.org/jdk/pull/22769#discussion_r1887370371
