On Thu, 2 May 2024 13:49:43 GMT, Hannes Wallnöfer <hann...@openjdk.org> wrote:
>> Nizar Benalla has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> - Added some legacy modules that existed long before preview features
>> (they were incubating)
>> - Not checking elements enclosed withing a record
>> - Only check if the file is readable using `Files.isReadable`
>> - Dropped the use of `Files.exists` and `Files.isDirectory`
>> - Use `--add-modules` option now to resolve certain modules
>
> test/jdk/tools/sincechecker/SinceChecker.java line 51:
>
>> 49:
>> 50: /*
>> 51: - This checker checks the values of the `@since` tag found in the
>> documentation comment for an element against the release in which the
>> element first appeared.
>
> In addition to the long lines mentioned by Jon which make the comments hard
> to read, I find it strange that every sentence is formatted as a list item
> with a leading dash. I think it's ok when describing different parts/steps of
> the algorithm, but at least the first sentence in the comment should not be a
> list item IMO.
Fixed, thanks.
> test/jdk/tools/sincechecker/SinceChecker.java line 216:
>
>> 214: srcZip =
>> testJdk.getParent().resolve("images").resolve("jdk").resolve("lib").resolve("src.zip");
>> 215: }
>> 216: if (!Files.exists(srcZip) && !Files.isDirectory(srcZip)) {
>
> This condition looks wrong. If `exists` returns false, it also implies that
> `isDirectory` returns false. I think there's no need to check for a
> (not-)directory here.
Fixed in
[3f226ef](https://github.com/openjdk/jdk/pull/18934/commits/3f226ef9134b71a1b63b6562b8381be909c30343),
I now only check if the file is readable.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/18934#discussion_r1590327624
PR Review Comment: https://git.openjdk.org/jdk/pull/18934#discussion_r1590327722
