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

[Peter Levart]

Hi,

On 3/11/19 5:29 PM, Joe Darcy wrote:
> 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 
> + "]");

...and even if it were effectively final, the use of that method would 
not help in preventing the construction of garbage. It would just 
replace one type of garbage (StringBuilder(s) and concatenated 
String(s)) with another type (capturing lambda instances).

So this is best left as is.

+1

Peter

>
> 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