On Tue, 13 May 2025 13:56:34 GMT, Jaikiran Pai <j...@openjdk.org> wrote:
>> David Beaumont has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Fix comment based on current behaviour.
>
> src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java line 160:
>
>> 158:
>> 159: // Parses the file system permission from an environmental
>> parameter. While
>> 160: // the FileSystemAccessMode is private, we don't need to check
>> if it was
>
> The second sentence perhaps is a leftover? I don't see any
> `FileSystemAccessMode` type. I think it would be better to remove that second
> sentence altogether. The rest of the comment looks good and clearly states
> what this method does.
Done.
> src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java line 167:
>
>> 165: return null;
>> 166: }
>> 167: case String label when READ_WRITE.label.equals(label)
>> -> {
>
> I haven't yet fully caught up on the newer features of switch/case. Does this
> have any advantage as compared to the simpler:
>
>
> case "readWrite" -> {
> return READ_WRITE;
> }
> case "readOnly" -> {
> return READ_ONLY;
> }
Or just an if :)
The reason for the new style was the "need" for the "String label when"
qualifier, which I don't actually need because String.equals(xx) copes with
being given non-strings fine. You can't use the syntax you suggested in the new
switch since "value" isn't a String.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2091667975
PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2091665729
