On Wed, 10 Mar 2021 18:16:49 GMT, Andrey Turbanov <github.com+741251+turban...@openjdk.org> wrote:
>>> Nice catch! >>> >>> We have a test that is supposed to test this: >>> https://github.com/openjdk/jdk/blob/master/test/jdk/java/foreign/TestNulls.java >>> But it is only checking if an NPE is thrown, and not checking for a >>> message, since `Objects::requireNonNull` does not set an exception message. >>> I guess that test was still passing because NPEs are thrown at some other >>> point during the call. >> >> Right, checked that passes (had only run the java/lang/invoke tests >> locally). An alternative approach here is to verify all methods already >> implicitly check null and remove all these, but being explicit is nice and >> reduces possibility of surprise. > > I found few more similar places in JFR code, where `Objects.nonNull` is used > incorrectly. > jdk.jfr.internal.consumer.AbstractEventStream#setStartTime > jdk.jfr.consumer.EventStream#openRepository(java.nio.file.Path) > jdk.jfr.Recording#setFlushInterval Created an issue: JDK-8263395: Incorrect use of Objects.nonNull > I found few more similar places in JFR code, where `Objects.nonNull` is used > incorrectly. > > ``` > jdk.jfr.internal.consumer.AbstractEventStream#setStartTime > jdk.jfr.consumer.EventStream#openRepository(java.nio.file.Path) > jdk.jfr.Recording#setFlushInterval > ``` ------------- PR: https://git.openjdk.java.net/jdk/pull/2914
