Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Handle assertThat(...).isNotNull() statements #304
Handle assertThat(...).isNotNull() statements #304
Changes from 5 commits
af56ff8
46b5d98
998bddb
ffe2698
c93b469
7e1fd8b
feb87b4
7eb7a91
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, upon reflection, this whole thing is not thread safe, since this could execute concurrent with
initializeMethodNames
and returntrue
when sayisNotNullOwner
and the rest are still uninitialized. So the other method beingsynchronized
is mostly pointless.My thoughts:
initializeMethodNames
and just have that method return early ifisNotNull
is already set, then callinitializeMethodNames
unconditionally fromonDataflowVisitMethodInvocation
toString()
to match the methods, measure the overhead and leave it at that if it's not high. Early optimization being the root of all evil and all that :)What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concurrency is only an issue with static fields, which we should avoid unless they are immutable values. There should be a single BugChecker instance per javac instance and it should not be getting called from multiple threads.
Since the only static fields here are final Strings we should be fine. But the
synchronized
on the private method is unnecessary.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the clarification. Removed
synchronized
on the method.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After adding the flag, do add a positive test that returns an error on the same code with the flag turned off.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a test with flag turned off.