-
Notifications
You must be signed in to change notification settings - Fork 221
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
Upgrade various dependencies #1313
Conversation
Also use this updated formatter to bulk reformat all Java code.
@@ -204,6 +204,7 @@ private static StateMachineFactory<IFlowLabel> makeStateMachineFactory() { | |||
return new ContextSensitiveStateMachine.Factory(); | |||
} | |||
|
|||
@SuppressWarnings("ComparisonOutOfRange") |
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.
This is a new ERROR-level check and usually it's best not to just suppress these. Did you look at the error message and conclude these were false positives? At the least we should add a comment. If we're not sure if the warnings are false positives, we should file an issue to look into them more, or maybe split out the Error Prone upgrade into a separate PR so we can upgrade and fix the issues at the same time.
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.
Did you look at the error message and conclude these were false positives?
I did. In each case, the code in question is comparing the current count or size of something to a static final int
limit, where that limit is set to Integer.MAX_VALUE
. It appears that the intent is to have no limit by default, but also to make it easy to impose a limit by changing how the static final int
is initialized. Here's a typical example:
private static final int MAX_SIZE = Integer.MAX_VALUE;
Latent limit check that is always true
unless the initialization of MAX_SIZE
is changed:
if (size <= MAX_SIZE) {
I believe that these checks are correct when the limit is set to something below Integer.MAX_VALUE
. I also believe that they intentionally become vacuous in the default case when the limit is Integer.MAX_VALUE
. So the warnings are correct in that each check is either always true or always false, but that is by design.
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 explanation! I wish we didn't have to suppress this error-level check, but I'm not sure there is a better way to do things. I guess we could set MAX_SIZE
to Integer.MAX_VALUE - 1
, which would make the check non-vacuous, or have a separate boolean variable to indicate whether a size limit is in place. But both of those options seem like overkill for this issue.
For now, could you add a short comment in each spot explaining the suppression?
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.
44df486 improves our handling of these limit checks as follows:
- In
DemandCastChecker
, add the twoint
s in a way that avoidsint
overflow. - In
ImmutableStack
, call a new helper function that centralizes the suppression and explains what's going on.
6a6e14d
to
7635469
Compare
7635469
to
3e034cf
Compare
3e034cf
to
44df486
Compare
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 these updates!
gradle/libs.versions.toml
:gradle/libs.versions.toml
plus trivial (whitespace-only) changes elsewhere:gradle/libs.versions.toml
plus nontrivial changes elsewhere: