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

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 2 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
16 changes: 7 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,11 @@ 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 ($members[$p].kind == "ARRAY")
if ($p == null) {
throw new NullPointerException("Null $p");
throw new NullPointerException("Null $p");
}

#end

#if ($members[$p].kind == "ARRAY")
#if ($params[$p].kind == "ARRAY")

this.$p = #cloneArray(${p});
Expand All @@ -99,9 +95,11 @@ final class $className implements $annotationName, `java.io.Serializable` {

#end
#else

this.$p = $p;

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

Expand Down
10 changes: 4 additions & 6 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 @@ -75,16 +75,14 @@ ${modifiers}class $subclass$formalTypes extends $origClass$actualTypes {
## the constructor is called from the extension code.

#if ($identifiers)
if ($p == null) {
throw new NullPointerException("Null $p.name");
}
this.$p = `java.util.Objects`.requireNonNull($p, "Null $p");
Copy link
Member

Choose a reason for hiding this comment

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

This change makes sense. However, the indentation convention in this file is that Velocity directives (like #if) and code lines are indented independently of each other. So this line should be indented the same as the if statement it replaces.

(It happens that the code indentation doesn't matter because a separate indentation pass will fix it up anyway, but I think following the conventions makes the code easier to follow.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed indentation. Please let me know if I missed something.

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

#end

#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 = $p;
#end
#end
}

Expand Down
6 changes: 2 additions & 4 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
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