On Thu, 9 Apr 2026 12:32:11 GMT, Chen Liang <[email protected]> wrote:
> I recommend adding some comments here about the hierarchy - the UTF8 is
> supported by the base class as the zip format requires some part of it to be
> handled by utf8, etc.
>
> In particular, note that ZipCoder instances in general are still stateful.
I have pushed an update to the class description. Please have a look.
> We don't need this guy. Just use the `String.getBytes(UTF_8)` and `new
> String(bytes, UTF_8)` instead.
While this would be okay for `getBytes` (a String is always encodable to
UTF-8), I don't think it holds for toString. We need to throw
`IllegalArgumentException` for decoding failures (the byte array could hold
invalid byte sequences).
Regardless, since this is a refactoring PR, I think I would prefer to avoid
introducing changes and improvements that are not strictly related to the
hierarchy change. I feel it would be cleaner to defer such enhancements to a
follow up (there are some other cleanups I would may want to propopse to align
ZipFS and java.util.zip versions).
What do you think?
> src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipCoder.java line 121:
>
>> 119: @Override
>> 120: boolean isUTF8() {
>> 121: return cs == UTF_8;
>
> Suggestion:
>
> return false;
As long as we have the UTF-8 CharsetEncoder instance, this is needed.
I know the UTF-8 CharsetEncoder instance looks a bit awkward. In
`java.util.zip` this is not needed because the base class here has access to
`JavaLangAccess` secrets. In ZipFS we don't. Before this change, we could just
call super, which we now can't. So the only easy solution which came to me was
to delegate to a UTF8 CharsetZipCoder as a non-ascii fallback.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/30640#discussion_r3060206998
PR Review Comment: https://git.openjdk.org/jdk/pull/30640#discussion_r3060203794
PR Review Comment: https://git.openjdk.org/jdk/pull/30640#discussion_r3060220844
