On Tue, 8 Apr 2025 11:45:49 GMT, Jorn Vernee <[email protected]> wrote:
>> src/jdk.jdeps/share/classes/com/sun/tools/jnativescan/NativeMethodFinder.java
>> line 91:
>>
>>> 89: err.println("Error while processing
>>> method: " +
>>> 90: MethodRef.ofModel(methodModel) +
>>> ": " + e.getMessage());
>>> 91: }
>>
>> This catch block is per-method. It means that if the rest of the method
>> contained references to restricted methods, we would not see them. The catch
>> block should be moved to be just around the call to `isRestricted`, which
>> can fail. Then we can record the error and keep iterating over the other
>> instructions.
>
> Also, instead of passing `err` down to this code, I think we should define an
> interface for `NativeMethodFinder` to log diagnostics instead. e.g. something
> like:
>
>
> interface Diagnostics {
> void error(MethodRef context, JNativeScanFatalError error);
> }
>
>
> `JNativeScanTask` can then define the implementation how it wants. The
> benefit of that is that we have more freedom to print the errors how we want.
> For instance, I think we should first collect all the errors, and
> de-duplicate error messages before printing them, so that if there are 10
> references to the same system class, we only print the error message once,
> and we can print a header like: `Errors while processing classes: ...`
> This catch block is per-method. It means that if the rest of the method
> contained references to restricted methods, we would not see them. The catch
> block should be moved to be just around the call to isRestricted, which can
> fail. Then we can record the error and keep iterating over the other
> instructions.
That makes a lot of sense. I will update it.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/24499#discussion_r2033030209
