On Fri, 17 May 2024 14:49:46 GMT, Jan Lahoda <jlah...@openjdk.org> wrote:
>> Nizar Benalla has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> - Not checking elements enclosed within a record, I doubt a record class
>> will change after being created
>> - Added a null check as `toString` can return an exception
>
> test/jdk/tools/sincechecker/SinceChecker.java line 224:
>
>> 222: }
>> 223:
>> 224: return LEGACY_PREVIEW_METHODS.containsKey(currentVersion)
>
> Nit: could probably be `LEGACY_PREVIEW_METHODS.getOrDefault(currentVersion,
> Map.of()).contains(uniqueId)`
Fixed, thanks.
> test/jdk/tools/sincechecker/SinceChecker.java line 309:
>
>> 307: error("module-info.java not found or couldn't be opened
>> AND this module has no unqualified exports");
>> 308: } catch (NullPointerException e) {
>> 309: error("module-info.java does not contain an `@since`");
>
> Catching a NPE like this feels like a code smell to me. I assume it may
> happen when `extractSinceVersionFromText(moduleInfoContent)` returns `null` -
> so just store that in a variable, and check for `null` there.
Fixed in
[e82dfbf](https://github.com/openjdk/jdk/pull/18934/commits/e82dfbf0f3df1dfcb1e482aac09fb34e39af9be0),
I tried not catching NPE before but found the extra checks a bit ugly.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/18934#discussion_r1605898860
PR Review Comment: https://git.openjdk.org/jdk/pull/18934#discussion_r1605898785
