On Tue, 8 Apr 2025 12:00:41 GMT, Danish Nawab <d...@openjdk.org> wrote:
>>> 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? The buffering shouldn't be hard to achieve? `JNativeScanTask::run` can just define a `Set<String>` to which the error messages are added by the diagnostics callback. I don't see why `Diagnostics` would need to be `AutoCloseable`. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/24499#discussion_r2033053788
