Hi Mike, as described in the bug comments, I still think that correctly handling a null Supplier is the cleanest and easiest solution. Without the change our VM will throw the following NPE:
java.lang.NullPointerException: while trying to invoke the method java.util.function.Supplier.get() of a null object loaded from the second parameter of the method at java.util.Objects.requireNonNull(Objects.java:290) which is more descriptive and perfectly legal, but it may be not what the user expects (i.e. a NPE with a null message). Anyhow, I don't insist on my change. If everybody agrees that it would be better to change the documentation then please go ahead. But keep in mind that Joe also had some objections against the documentation changes in the bug comments. Regards, Volker On Tue, Feb 11, 2014 at 12:24 AM, Mike Duigou <mike.dui...@oracle.com> wrote: > I would prefer to leave the implementation as is and amend the documentation. > The difference in behaviour between the overloads seems OK since it will > still be valid for the Supplier to return null. The String overload > reasonably allows null since the Throwable(String) constructor allows null > message. (See Throwable.getMessage()). > > It seems like ignoring/masking the likely error of passing a null Supplier > isn't really being helpful. > > YMMV, > > Mike > > On Feb 10 2014, at 05:30 , Volker Simonis <volker.simo...@gmail.com> wrote: > >> Hi, >> >> could you please review the following tiny change which fixes a >> problem in Objects.requireNonNull: >> >> http://cr.openjdk.java.net/~simonis/webrevs/8033909/ >> https://bugs.openjdk.java.net/browse/JDK-8033909 >> >> The problem is that Objects.requireNonNull(T, Supplier) does not check >> for the Supplier argument being null. Instead it relies on the fact, >> that the VM will implicitly throw a NullPointerException while trying >> to call .get on the Supplier argument during the creation of the >> explicit NullPointerException which is supposed to be thrown by >> Objects.requireNonNull(T, Supplier) for a null T argument. >> >> This makes the behavior of Objects.requireNonNull(T, Supplier) >> slightly different from the one of the overladed >> Objects.requireNonNull(T, String) version. The latter one always >> throws a NPE with a null message in the case where the String argument >> is null. For the first one, it depends on the implementation of the VM >> whether the trown NPE will have a null message or not (i.e. the VM is >> free to create a NPE with an arbitrary message). >> >> The bug report mentions that this behavior makes it hard to develop a >> conformance test for the scenario and suggests to tweak the JavaDoc of >> Objects.requireNonNull(T, Supplier) such that it allows NPE with an >> unspecified message. >> >> However, I think the easiest fix is to just check for the supplier >> object beeing null and explicitely creating a NPE with a null message >> in that case. This will align the behavior of >> Objects.requireNonNull(T, Supplier) with that of >> Objects.requireNonNull(T, String). Also notice that the extra >> null-check is only performed if the object argument T of >> Objects.requireNonNull(T, Supplier) is null, which should be the >> unusual case anyway. I therefore don't expect this change to have any >> negative performance impact. >> >> This webrev is for jdk9, but I think it should be also downported to jdk8. >> >> Thank you and best regards, >> Volker >
