JDK 8 code review request for 8011800: Add java.util.Objects.requireNonNull(T, Supplier<String>)

[Joe Darcy]

Hello,

On 04/10/2013 05:01 AM, Alan Bateman wrote:
> On 09/04/2013 22:12, Joe Darcy wrote:
>> Hello,
>>
>> Please review my changes for
>>
>>     8011800: Add java.util.Objects.requireNonNull(T, Supplier<String>)
>>     http://cr.openjdk.java.net/~darcy/8011800.0/
>>
>> which add a new method to java.util.Objects to take a 
>> Supplier<String> rather than a String.
>>
>> Patch inline below.
>>
>> Thanks,
>>
>> -Joe
>>
> A typo in the javadoc "this methods allows" -> "this method allows".
>
> A subjective comment, but I would drop the word "sibling" from the 
> statement.
>
> A minor nit with the @param spilling over into a second line is that 
> it might be clearer to indent it so that it's clear where the next tag 
> starts. I see the existing requiresNonNull are inconsistent on this 
> point.
>
> The uninteresting Supplier is null case isn't specified, perhaps this 
> is deliberate?

That is deliberate. Instead of a body like

if (obj == null)
      throw new NullPointerException(messageSupplier.get());
  return obj;

I briefly considered something more elaborate like

if (obj == null)
      throw new 
NullPointerException(requireNonNull(messageSupplier.get(), "snarky 
comment"));
  return obj;

but I figured if you pass in a null message supplier, you get what you 
deserve and it wasn't worth the extra cost of bloating the method body 
and interfering with inlining.

>
> A typo in the test at line 208, "rvariant" -> "variant".
>
> Also the printed message at line 226 when you get don't pantaloons 
> should say "Supplier" rather than "2-arg".
>

I've reworded the specification:

     /**
      * Checks that the specified object reference is not {@code null} and
      * throws a customized {@link NullPointerException} if it is.
      *
      * <p>Unlike the method {@link requireNonNull(Object, String},
      * this method allows creation of the message to be deferred until
      * after the null check is made. While this may confer a
      * performance advantage in the non-null case, when deciding to
      * call this method care should be taken that the costs of
      * creating the message supplier are less than the cost of just
      * creating the string message directly.
      *
      * @param obj     the object reference to check for nullity
      * @param messageSupplier supplier of the detail message to be
      * used in the event that a {@code NullPointerException} is thrown
      * @param <T> the type of the reference
      * @return {@code obj} if not {@code null}
      * @throws NullPointerException if {@code obj} is {@code null}
      * @since 1.8
      */

and cleaned up the regression tests to use lambda expressions.  New 
webrev at

     http://cr.openjdk.java.net/~darcy/8011800.1/

Thanks,

-Joe