On Thu, 17 Apr 2025 16:44:02 GMT, Henry Jen <henry...@openjdk.org> wrote:
>> Chen Liang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Review remarks
>
> src/jdk.jdeps/share/classes/com/sun/tools/jdeps/ClassFileReader.java line 105:
>
>> 103:
>> 104: protected void skipEntry(ClassFileError ex, String fileName) {
>> 105: skippedEntries.add(String.format("%s: %s", ex.toString(),
>> fileName));
>
> The second parameter is not always a straightforward filename, consider to
> rename it, perhaps `entryPath`?
Indeed. Renamed to `entryPath`. Also adjusted this to accept `Throwable`, so we
can skip an entry due to either IOException or ClassFileError.
> src/jdk.jdeps/share/classes/com/sun/tools/jdeps/ClassFileReader.java line 237:
>
>> 235: skipEntry(ex, e.toString());
>> 236: } catch (IOException ex) {
>> 237: throw new UncheckedIOException(ex);
>
> Just to point out this was leading to ClassFileError in the old
> implementation, new implementation will relay the IOException, which I think
> is proper.
I have updated this to skip as well - if the outer structure is malformed, we
should not have reached this far and would have encountered IOException earlier.
> src/jdk.jdeps/share/classes/com/sun/tools/jdeps/ClassFileReader.java line 339:
>
>> 337: });
>> 338: } catch (UncheckedIOException ex) {
>> 339: throw ex.getCause();
>
> IOException used to skip entry with message and continue, the new behavior
> would change. I am not certain this is behavior compatible.
> I doubt IOException would be recoverable on different entry, but it might in
> rare cases?
Yep, I think skipping for IOE is correct. If IOE is serious enough that no
element is accessible, it should usually be thrown earlier.
> src/jdk.jdeps/share/classes/com/sun/tools/jdeps/DependencyFinder.java line
> 176:
>
>> 174: FutureTask<Set<Location>> task = new FutureTask<>(() -> {
>> 175: Set<Location> targets = new HashSet<>();
>> 176: archive.reader().processClassFiles(cf -> {
>
> I prefer the naming convention with forEach for operation to iterate through
> rather than have a `process` on a reader. Perhaps named like
> `forEachClassFile`?
Done in the update.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/24604#discussion_r2049625074
PR Review Comment: https://git.openjdk.org/jdk/pull/24604#discussion_r2049626124
PR Review Comment: https://git.openjdk.org/jdk/pull/24604#discussion_r2049626614
PR Review Comment: https://git.openjdk.org/jdk/pull/24604#discussion_r2049625262
