Skip to content

Commit 2dd82ad

Browse files
cpovirkGoogle Java Core Libraries
authored andcommitted
Refactor the main AbstractFuture implementation to prepare for using it for more environments.
That includes J2KT and [GWT/J2CL](#2934). Changes include: - Suppress nullness at smaller scopes, fixing errors that were hidden by the old, broad suppression. - Remove `synchronized` from an override of `fillInStackTrace`. J2KT doesn't like `synchronized` except on very specific types, and we don't need it. - Introduce `rethrowIfErrorOtherThanStackOverflow` and `interruptCurrentThread`, `Platform` methods that will require different implementations for J2KT and J2CL/GWT. - Introduce helper methods like `casListeners(expect, update)` for `AtomicHelper` operations. These reduce verbosity relative to `ATOMIC_HELPER.casListeners(this, expect, update)`. They also prepare for `AbstractFuture` implementations that don't use `AtomicHelper`. - Introduce `notInstanceOfSetFuture`. This is arguably nicer than `!(localValue instanceof SetFuture)`, but the real purpose is to prepare for when code in a different file needs to check `instance SetFuture`. (I could be convinced that I should just make `SetFuture` package-private instead, even though no one outside the file should use it for anything but an `instanceof` check.) - Mysteriously move things around, increase visibility of members, and introduce and sometimes use accessors. This is to prepare for when some of the code will be moving to a separate file so that the remainder of `AbstractFuture` can be wholly shared across different platforms. - Fix a few typos in comments. Also, rename `SetFuture`. This isn't directly related, but now is as good a time as any to do it. Additional bonus: This CL probably makes [the logging at the "end" of `AbstractFuture` static initialization](https://github.com/google/guava/blob/7ec362ec68b630363231d5292cd6b2577c710be6/guava/src/com/google/common/util/concurrent/AbstractFuture.java#L210) have a better chance of actually working in the hypothetical situation that a logger uses `AbstractFuture`: Currently, `AbstractFuture` performs some further initialization _after_ that logging (such as the initialization of `NULL`). Now, it performs all that initialization before the `static` block that might log. RELNOTES=n/a PiperOrigin-RevId: 729313044
1 parent ba42c96 commit 2dd82ad

File tree

7 files changed

+836
-587
lines changed

7 files changed

+836
-587
lines changed

android/guava-testlib/src/com/google/common/testing/NullPointerTester.java

Lines changed: 27 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818

1919
import static com.google.common.base.Preconditions.checkArgument;
2020
import static com.google.common.base.Preconditions.checkNotNull;
21+
import static java.util.Arrays.stream;
2122

2223
import com.google.common.annotations.GwtIncompatible;
2324
import com.google.common.annotations.J2ktIncompatible;
@@ -33,6 +34,7 @@
3334
import com.google.common.reflect.Parameter;
3435
import com.google.common.reflect.Reflection;
3536
import com.google.common.reflect.TypeToken;
37+
import com.google.common.util.concurrent.AbstractFuture;
3638
import com.google.errorprone.annotations.CanIgnoreReturnValue;
3739
import java.lang.annotation.Annotation;
3840
import java.lang.reflect.Constructor;
@@ -76,6 +78,12 @@ public final class NullPointerTester {
7678

7779
private ExceptionTypePolicy policy = ExceptionTypePolicy.NPE_OR_UOE;
7880

81+
/*
82+
* Requiring desugaring for guava-*testlib* is likely safe, at least for the reflection-based
83+
* NullPointerTester. But if you are a user who is reading this because this change caused you
84+
* trouble, please let us know: https://github.com/google/guava/issues/new
85+
*/
86+
@IgnoreJRERequirement
7987
public NullPointerTester() {
8088
try {
8189
/*
@@ -88,8 +96,26 @@ public NullPointerTester() {
8896
*/
8997
ignoredMembers.add(Converter.class.getMethod("apply", Object.class));
9098
} catch (NoSuchMethodException shouldBeImpossible) {
91-
// OK, fine: If it doesn't exist, then there's chance that we're going to be asked to test it.
99+
// Fine: If it doesn't exist, then there's no chance that we're going to be asked to test it.
92100
}
101+
102+
/*
103+
* These methods "should" call checkNotNull. However, I'm wary of accidentally introducing
104+
* anything that might slow down execution on such a hot path. Given that the methods are only
105+
* package-private, I feel OK with just not testing them for NPE.
106+
*
107+
* Note that testing casValue is particularly dangerous because it uses Unsafe under some
108+
* versions of Java, and apparently Unsafe can cause SIGSEGV instead of NPE—almost as if it's
109+
* not safe.
110+
*/
111+
stream(AbstractFuture.class.getDeclaredMethods())
112+
.filter(
113+
m ->
114+
m.getName().equals("getDoneValue")
115+
|| m.getName().equals("casValue")
116+
|| m.getName().equals("casListeners")
117+
|| m.getName().equals("gasListeners"))
118+
.forEach(ignoredMembers::add);
93119
}
94120

95121
/**

0 commit comments

Comments
 (0)