On Thu, 22 May 2025 01:41:51 GMT, Henry Jen <henry...@openjdk.org> wrote:
>> 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.
Thank you for the update to this method, Henry. Retaining the use of `File` is
fine. We have a separate plan for future to use `java.nio.file.Path` for the
code in the jar tool and this can be revisited at that time.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/24430#discussion_r2102239606
