JDK 13 RFR of JDK-8220346: Refactor java.lang.Throwable to use Objects.requireNonNull

[Joe Darcy]

Hello,

Always surprising how much discussion an (apparently) simple refactoring 
can generate!

Thanks to Tagir for spotting this issue.

For completeness, the two-argument forms of requireNonNull which takes a 
Supplier<String> is not applicable to preserve the message behavior 
since the loop counter i is not final or effectively final:

     Objects.requireNonNull(defensiveCopy[i],  () -> "stackTrace[" + i + 
"]");

Therefore, I'll revert this use of requireNonNull in Throwable but keep 
the other three:

diff -r 289fd6cb7480 src/java.base/share/classes/java/lang/Throwable.java
--- a/src/java.base/share/classes/java/lang/Throwable.java    Mon Mar 11 
15:34:16 2019 +0100
+++ b/src/java.base/share/classes/java/lang/Throwable.java    Mon Mar 11 
09:28:51 2019 -0700
@@ -914,8 +914,7 @@
                  for (Throwable t : suppressedExceptions) {
                      // Enforce constraints on suppressed exceptions in
                      // case of corrupt or malicious stream.
-                    if (t == null)
-                        throw new NullPointerException(NULL_CAUSE_MESSAGE);
+                    Objects.requireNonNull(t, NULL_CAUSE_MESSAGE);
                      if (t == this)
                          throw new 
IllegalArgumentException(SELF_SUPPRESSION_MESSAGE);
                      suppressed.add(t);
@@ -942,8 +941,7 @@
                  stackTrace = null;
              } else { // Verify stack trace elements are non-null.
                  for(StackTraceElement ste : stackTrace) {
-                    if (ste == null)
-                        throw new NullPointerException("null 
StackTraceElement in serial stream. ");
+                    Objects.requireNonNull(ste, "null StackTraceElement 
in serial stream.");
                  }
              }
          } else {
@@ -1034,8 +1032,7 @@
          if (exception == this)
              throw new 
IllegalArgumentException(SELF_SUPPRESSION_MESSAGE, exception);

-        if (exception == null)
-            throw new NullPointerException(NULL_CAUSE_MESSAGE);
+        Objects.requireNonNull(exception, NULL_CAUSE_MESSAGE);

          if (suppressedExceptions == null) // Suppressed exceptions not 
recorded
              return;

Thanks,

-Joe

On 3/10/2019 4:21 AM, Remi Forax wrote:
> ----- Mail original -----
>> De: "Martin Buchholz" <martinrb at google.com>
>> À: "Tagir Valeev" <amaembo at gmail.com>
>> Cc: "core-libs-dev" <core-libs-dev at openjdk.java.net>
>> Envoyé: Vendredi 8 Mars 2019 21:35:59
>> Objet: Re: JDK 13 RFR of JDK-8220346: Refactor java.lang.Throwable to use Objects.requireNonNull
>> On Fri, Mar 8, 2019 at 3:57 AM Tagir Valeev <amaembo at gmail.com> wrote:
>>
>>> Hello!
>>>
>>>> diff -r 274361bd6915 src/java.base/share/classes/java/lang/Throwable.java
>>>> --- a/src/java.base/share/classes/java/lang/Throwable.java    Thu Mar 07
>>>> 10:22:19 2019 +0100
>>>> +++ b/src/java.base/share/classes/java/lang/Throwable.java    Fri Mar 08
>>>> 02:06:42 2019 -0800
>>>> @@ -874,8 +874,7 @@
>>>>            // Validate argument
>>>>            StackTraceElement[] defensiveCopy = stackTrace.clone();
>>>>            for (int i = 0; i < defensiveCopy.length; i++) {
>>>> -            if (defensiveCopy[i] == null)
>>>> -                throw new NullPointerException("stackTrace[" + i + "]");
>>>> +            Objects.requireNonNull(defensiveCopy[i], "stackTrace[" + i
>>>> + "]");
>>> Won't it produce unnecessary garbage due to string concatenation on
>>> the common case when all frames are non-null?
>>>
>> I share Tagir's doubts.  I avoid this construction in performance sensitive
>> code.
>> Java doesn't offer syntactic abstraction (can I haz macros?) so the Java
>> Way is to write the explicit if.
>>
>> Logging frameworks have the same trouble.
>> https://github.com/google/flogger
>
> No, they don't if the API use the pattern described by Peter
> https://github.com/forax/beautiful_logger
>
>> Perhaps hotspot's improved automatic NPE reporting makes the message detail
>> here obsolete?
> anyway, i agree with what's Martin and Tagir said, requireNonNull() should not be used in this particular case.
>
> Rémi