On Wed, 10 Mar 2021 16:40:56 GMT, Claes Redestad <redes...@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. > >> 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 ------------- PR: https://git.openjdk.java.net/jdk/pull/2914
