RFR: 8350880: (zipfs) Add support for read-only zip file systems [v3]
David Beaumont
duke at openjdk.org
Thu May 15 17:18:56 UTC 2025
On Tue, 13 May 2025 12:32:56 GMT, Jaikiran Pai <jpai at 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 147:
>
>> 145: // this could exist somewhere common, but if it's definitely never going to
>> 146: // be shared, it could be made public here.
>> 147: private enum AccessMode {
>
> Hello David, I think having it `private` is fine and in fact more appropriate. I would suggest removing the comment though.
Done. And if it's not getting made visible (i.e. values are always strings) I can simplify things too.
> src/jdk.zipfs/share/classes/module-info.java line 277:
>
>> 275: * <td>
>> 276: * A value defining the desired read/write access mode of the file system
>> 277: * (either <em>read-write</em> or <em>read-only</em>).
>
> This line is slightly confusing. Initially I thought `read-write` and `read-only` are the actual values that this environment property will accept. But further review suggests that the actual literals supported are `readOnly` and `readWrite` and this line here I think is trying to explain the supported semantics of the file system.
>
> Perhaps we could reword this to something like:
>
>>
>> A value defining the desired access mode of the file system. A ZIP file system can be created to allow for read-write access or read-only access.
Yeah, I tried to distinguish the accessMode flags (for which there are 3 states, ro, rw, n/a) and the final state of the file system from fs.isReadOnly(). Hence using `<em>...</em>` for whenever the actually resulting state is mentioned. I guess it wasn't clear enough.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2091645807
PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2091649006
More information about the core-libs-dev
mailing list