On Wed, 21 May 2025 16:53:17 GMT, Jaikiran Pai <[email protected]> wrote:
>> Henry Jen has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Mismatched order is considered invalid
>
> src/jdk.jartool/share/classes/sun/tools/jar/Main.java line 454:
>
>> 452: new FileInputStream(file)))) {
>> 453:
>> 454: return Validator.validate(this, zf, zis);
>
> I think it might be better to change the `Validator.validate()` method to
> accept a `java.nio.file.Path` instead of accepting a ZipFile instance and a
> ZipInputStream instance. In its current form it feels odd that the call site
> (like here) needs to pass ZipFile and ZipInputStream to a validator. The
> checks using a ZipFile and a ZipInputStream for the same file is more an
> internal detail of the validator, so it would be good to let it construct
> those instances internally as appropriate.
>
> The `Validator` class itself belongs to an internal package of this tool, so
> changing the signature of this method to accept a `Path` wouldn't be a
> problem.
>
> It also looks like the Validator uses this `Main` instance merely for error
> reporting. So maybe in a future update we could remove the need to pass along
> this `Main` instance to the validator. That one doesn't have to be done in
> this PR.
I keep File instead of Path to avoid unnecessary changes.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/24430#discussion_r2101460623
