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

[Tagir Valeev]

Looks good now, thanks!

With best regards,
Tagir Valeev

On Mon, Mar 11, 2019 at 11:29 PM Joe Darcy <joe.darcy at oracle.com> 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 +
> "]");
>
> 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