On Wed, 30 Oct 2024 16:17:20 GMT, Claes Redestad <redes...@openjdk.org> wrote:
>> src/java.base/share/classes/java/util/zip/ZipFile.java line 1798:
>>
>>> 1796:
>>> metaVersions.computeIfAbsent(hashCode, _ -> new BitSet()).set(version);
>>> 1797: } catch (Exception e) {
>>> 1798: throw new IllegalArgumentException(e);
>>
>> Hello Eirik, I'm late to this to PR. Was throwing an unspecified
>> `IllegalArgumentException` from here intentional? Should it have been
>> `IOException` instead?
>
> @eirbjo will know for sure but I think it's intentional since if there's an
> encoding error we'd have thrown earlier when hashing over the full name.
Hello Claes,
> if there's an encoding error we'd have thrown earlier when hashing over the
> full name.
If I understand correctly, then I think you are referring to the
`ZipCoder.checkedHash()` which throws that `Exception`.
Before the change in this PR, there was (and continues to exist) one such call
in the `checkAndAddEntry()` private method of this `Source` class. In that
place we catch the `Exception` and throw a `ZipException` (which is an
`IOException`)
https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/util/zip/ZipFile.java#L1264.
I think we should be doing the same here and throw a `ZipException` instead of
an `IllegalArgumentException`
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/21489#discussion_r1822997059
