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" <marti...@google.com>
À: "Tagir Valeev" <amae...@gmail.com>
Cc: "core-libs-dev" <core-libs-dev@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 <amae...@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
