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.

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

Conversation

reugn
Copy link
Contributor

@reugn reugn commented Nov 13, 2021

Generate cleaner, more concise sources.

@google-cla google-cla bot added the cla: no label Nov 13, 2021
@reugn
Copy link
Contributor Author

reugn commented Nov 13, 2021

Please rescan for CLA.

Copy link
Member

@eamonnmcmanus eamonnmcmanus left a comment

Choose a reason for hiding this comment

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

Thanks, this is helpful! In the Old Days we targeted Java 6 so we couldn't use java.util.Objects, but that hasn't been true for a while now, and we'd already started using it. We should certainly use it more consistently.

Using the value returned by requireNonNull leads to shorter code, though I think that doesn't really work out in autoannotation.vm.

@@ -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 ($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.

#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.

@google-cla google-cla bot added cla: yes and removed cla: no labels Nov 15, 2021
@eamonnmcmanus
Copy link
Member

Just an update here: when running this change against Google's internal codebase we found a number of problems with Android tooling. I think they can probably be fixed, but it may be a while.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants