On Tue, 8 Apr 2025 11:55:58 GMT, Danish Nawab <d...@openjdk.org> wrote:
>> 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.
> 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:
This makes sense. I can update it, accordingly.
Regarding deduplication: this means we would need to buffer the errors and
print them all at the end, which would require coordination between different
layers, and we might need to mark `Diagnostics` as `Closable/AutoClosable`.
Would it be ok for you, if the buffering/deduplication can be done a follow-up
and for now we just change the termination behavior? Or do you see that as an
essential component that should be included with this change?
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/24499#discussion_r2033037953
