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