Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use Objects.requireNonNull wherever possible #1200

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Use Objects.requireNonNull wherever possible #1200

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9e3f988
use `Objects.requireNonNull` wherever possible
reugn Nov 13, 2021
59b0a06
align tests with the fix
reugn Nov 13, 2021
266e658
fix indentation and invert if statement
reugn Nov 15, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 3 additions & 9 deletions value/src/main/java/com/google/auto/value/processor/autoannotation.vm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,15 +70,9 @@ final class $className implements $annotationName, `java.io.Serializable` {
$params[$p].type $members[$p] #if ($foreach.hasNext) , #end
#end ) {
#foreach ($p in $params.keySet())
#if (!$members[$p].kind.primitive)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand why the changes in this file need to be anything more than changing if (x == null)+throw new NPE(msg) into Objects.requireNonNull(x, msg). And, in fact the opportunity to do so on line 76 seems to have been missed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactored.


if ($p == null) {
throw new NullPointerException("Null $p");
}

#end

#if ($members[$p].kind == "ARRAY")
`java.util.Objects`.requireNonNull($p, "Null $p");
#if ($params[$p].kind == "ARRAY")

this.$p = #cloneArray(${p});
Expand All @@ -98,10 +92,10 @@ final class $className implements $annotationName, `java.io.Serializable` {
this.$p = ${p}.toArray(new ${members[$p].componentType}[0]);

#end
#elseif (!$members[$p].kind.primitive)
this.$p = `java.util.Objects`.requireNonNull($p, "Null $p");
#else

this.$p = $p;

#end
#end

Expand Down
27 changes: 11 additions & 16 deletions value/src/main/java/com/google/auto/value/processor/autovalue.vm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,23 +68,18 @@ ${modifiers}class $subclass$formalTypes extends $origClass$actualTypes {
${p.nullableAnnotation}$p.type $p #if ($foreach.hasNext) , #end
#end ) {
#foreach ($p in $props)
#if (!$p.kind.primitive && !$p.nullable && ($builderTypeName == "" || !$isFinal))
## We don't need a null check if the type is primitive or @Nullable. We also don't need it
## if there is a builder, since the build() method will check for us. However, if there is a
## builder but there are also extensions (!$isFinal) then we can't omit the null check because
## the constructor is called from the extension code.

#if ($identifiers)
if ($p == null) {
throw new NullPointerException("Null $p.name");
}
#else
`java.util.Objects`.requireNonNull($p);
#end

#if ($p.kind.primitive || $p.nullable || ($builderTypeName != "" && $isFinal))
## We don't need a null check if the type is primitive or @Nullable. We also don't need it
## if there is a builder, since the build() method will check for us. However, if there is a
## builder but there are also extensions (!$isFinal) then we can't omit the null check because
## the constructor is called from the extension code.

this.$p = $p;
#elseif ($identifiers)
this.$p = `java.util.Objects`.requireNonNull($p, "Null $p");
#else
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that there is now an #else, I might be inclined to invert the sense of the #if:

#if ($p.kind.primitive || $p.nullable || ($builderTypeName != "" && $isFinal)
## ...comment...
this.$p = $p;
#elseif ($identifiers)
this.$p = `java.util.Objects`.requireNonNull($p, "Null $p");
#else
this.$p = `java.util.Objects`.requireNonNull($p);
#end

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it looks better that way.

this.$p = `java.util.Objects`.requireNonNull($p);
#end

this.$p = $p;
#end
}

Expand Down
20 changes: 7 additions & 13 deletions value/src/test/java/com/google/auto/value/processor/AutoAnnotationCompilationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ public void testSimple() {
"import com.example.annotations.MyAnnotation;",
"import com.example.enums.MyEnum;",
"import java.io.Serializable;",
"import java.util.Objects;",
GeneratedImport.importGeneratedAnnotationType(),
"",
"@Generated(\"" + AutoAnnotationProcessor.class.getName() + "\")",
Expand All @@ -85,10 +86,7 @@ public void testSimple() {
" private static final int defaultedValue = 23;",
"",
" AutoAnnotation_AnnotationFactory_newMyAnnotation(MyEnum value) {",
" if (value == null) {",
" throw new NullPointerException(\"Null value\");",
" }",
" this.value = value;",
" this.value = Objects.requireNonNull(value, \"Null value\");",
" }",
"",
" @Override public Class<? extends MyAnnotation> annotationType() {",
Expand Down Expand Up @@ -245,6 +243,7 @@ public void testGwtSimple() {
"import com.example.annotations.MyAnnotation;",
"import java.io.Serializable",
"import java.util.Arrays;",
"import java.util.Objects;",
GeneratedImport.importGeneratedAnnotationType(),
"",
"@Generated(\"" + AutoAnnotationProcessor.class.getName() + "\")",
Expand All @@ -254,9 +253,7 @@ public void testGwtSimple() {
" private final int[] value;",
"",
" AutoAnnotation_AnnotationFactory_newMyAnnotation(int[] value) {",
" if (value == null) {",
" throw new NullPointerException(\"Null value\");",
" }",
" Objects.requireNonNull(value, \"Null value\");",
" this.value = Arrays.copyOf(value, value.length);",
" }",
"",
Expand Down Expand Up @@ -356,6 +353,7 @@ public void testCollectionsForArrays() {
"import java.util.Arrays;",
"import java.util.Collection;",
"import java.util.List;",
"import java.util.Objects;",
"import java.util.Set;",
GeneratedImport.importGeneratedAnnotationType(),
"",
Expand All @@ -369,13 +367,9 @@ public void testCollectionsForArrays() {
" AutoAnnotation_AnnotationFactory_newMyAnnotation(",
" List<Integer> value,",
" Set<MyEnum> enums) {",
" if (value == null) {",
" throw new NullPointerException(\"Null value\");",
" }",
" Objects.requireNonNull(value, \"Null value\");",
" this.value = intArrayFromCollection(value);",
" if (enums == null) {",
" throw new NullPointerException(\"Null enums\");",
" }",
" Objects.requireNonNull(enums, \"Null enums\");",
" this.enums = enums.toArray(new MyEnum[0];",
" }",
"",
Expand Down
17 changes: 5 additions & 12 deletions value/src/test/java/com/google/auto/value/processor/AutoValueCompilationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,7 @@ public void importTwoWays() {
"package foo.bar;",
"",
"import java.util.Arrays;",
"import java.util.Objects;",
GeneratedImport.importGeneratedAnnotationType(),
"",
"@Generated(\"" + AutoValueProcessor.class.getName() + "\")",
Expand All @@ -171,14 +172,8 @@ public void importTwoWays() {
" private final Arrays arrays;",
"",
" AutoValue_Baz(int[] ints, Arrays arrays) {",
" if (ints == null) {",
" throw new NullPointerException(\"Null ints\");",
" }",
" this.ints = ints;",
" if (arrays == null) {",
" throw new NullPointerException(\"Null arrays\");",
" }",
" this.arrays = arrays;",
" this.ints = Objects.requireNonNull(ints, \"Null ints\");",
" this.arrays = Objects.requireNonNull(arrays, \"Null arrays\");",
" }",
"",
" @SuppressWarnings(\"mutable\")",
Expand Down Expand Up @@ -301,6 +296,7 @@ public void testNestedParameterizedTypesWithTypeAnnotations() {
"",
"import foo.bar.Annot;",
"import foo.baz.OuterWithTypeParam;",
"import java.util.Objects;",
GeneratedImport.importGeneratedAnnotationType(),
"",
"@Generated(\"com.google.auto.value.processor.AutoValueProcessor\")",
Expand All @@ -311,10 +307,7 @@ public void testNestedParameterizedTypesWithTypeAnnotations() {
" AutoValue_Nesty(",
" @Annot(1) OuterWithTypeParam<@Annot(2) Double>"
+ ".@Annot(3) InnerWithTypeParam<@Annot(4) String> inner) {",
" if (inner == null) {",
" throw new NullPointerException(\"Null inner\");",
" }",
" this.inner = inner;",
" this.inner = Objects.requireNonNull(inner, \"Null inner\");",
" }",
"",
" @Override",
Expand Down
6 changes: 2 additions & 4 deletions value/src/test/java/com/google/auto/value/processor/ExtensionTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,7 @@ public void testExtensionConsumesProperties() {
"foo.bar.$AutoValue_Baz",
"package foo.bar;",
"",
"import java.util.Objects;",
GeneratedImport.importGeneratedAnnotationType(),
"",
"@Generated(\"com.google.auto.value.processor.AutoValueProcessor\")",
Expand All @@ -131,10 +132,7 @@ public void testExtensionConsumesProperties() {
"",
" $AutoValue_Baz(",
" String foo) {",
" if (foo == null) {",
" throw new NullPointerException(\"Null foo\");",
" }",
" this.foo = foo;",
" this.foo = Objects.requireNonNull(foo, \"Null foo\");",
" }",
"",
" @Override",
Expand Down