On Mon, 19 May 2025 12:24:28 GMT, Jaikiran Pai <j...@openjdk.org> wrote:
>> Hello David, I had another look at this code and after going through it, it
>> looked like `readOnly` field can in fact be made `final` because of the
>> refactoring changes that you did in this PR. I checked out your latest PR
>> locally and here's the additional change I did:
>>
>>
>> diff --git a/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java
>> b/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java
>> index e2fddd96fe8..f54b5360ac5 100644
>> --- a/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java
>> +++ b/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java
>> @@ -110,8 +110,7 @@ class ZipFileSystem extends FileSystem {
>> private final Path zfpath;
>> final ZipCoder zc;
>> private final ZipPath rootdir;
>> - // Starts in readOnly (safe mode), but might be reset at the end of
>> initialization.
>> - private boolean readOnly = true;
>> + private final boolean readOnly;
>>
>> // default time stamp for pseudo entries
>> private final long zfsDefaultTimeStamp = System.currentTimeMillis();
>> @@ -227,11 +226,6 @@ static ZipAccessMode from(Object value) {
>>
>> // Determining a release version uses 'this' instance to read paths
>> etc.
>> Optional<Integer> multiReleaseVersion =
>> determineReleaseVersion(env);
>> -
>> - // Set the version-based lookup function for multi-release JARs.
>> - this.entryLookup =
>> -
>> multiReleaseVersion.map(this::createVersionedLinks).orElse(Function.identity());
>> -
>> // We only allow read-write zip/jar files if they are not
>> multi-release
>> // JARs and the underlying file is writable.
>> this.readOnly = forceReadOnly || multiReleaseVersion.isPresent() ||
>> !Files.isWritable(zfpath);
>> @@ -241,6 +235,10 @@ static ZipAccessMode from(Object value) {
>> : "the ZIP file is not writable";
>> throw new IOException(reason);
>> }
>> + // Set the version-based lookup function for multi-release JARs.
>> + this.entryLookup =
>> +
>> multiReleaseVersion.map(this::createVersionedLinks).orElse(Function.identity());
>> +
>> }
>>
>> /**
>>
>>
>>
>>
>> With the refactoring changes you have done so far, we are now able to
>> determine the ultimate value for `readOnly` before anything in the
>> construction of `ZipFileSystem` would need access to it. Does this
>> additional change look reasonable to you? I haven't run any tests against
>> this change.
>>
>> Making it `final` and having...
>
> On second thought, may be this isn't enough. I see that I missed the fact
> that `determineReleaseVersion()` requires access to `this`. Please leave this
> in the current form that you have in your PR. I need a few more moments to
> consider if anything needs to be changed.
final members are good! Alternatively if making it final is not an option you
could consider replacing:
private boolean readOnly;
with
private boolean readWrite;
I looked at where `readOnly` was used and there are not that many places, so
inverting the flag might not be too intrusive.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2109210071
