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

Fix some compiler warnings in sdks/java/core #5319

Merged
merged 3 commits into from
May 16, 2018
Merged

Fix some compiler warnings in sdks/java/core #5319

merged 3 commits into from
May 16, 2018

Conversation

swegner
Copy link
Contributor

@swegner swegner commented May 9, 2018 •
edited
Loading

  1. This clears compiler warnings about SuppressFBWarnings for dependent libaries using the latest version:
warning: Cannot find annotation method 'value()' in type 'SuppressWarnings': 
class file for edu.umd.cs.findbugs.annotations.SuppressWarnings not found
  1. Manually fixed a bunch of ErrorProne compiler warnings in sdks/java/core.

The project is not yet ErrorProne-clean, but if we can get it there I recommend adding the -Werror compiler flag to treat warnings as errors.

@swegner
Copy link
Contributor Author

swegner commented May 9, 2018

Run Dataflow ValidatesRunner

@swegner swegner force-pushed the upgrade_fb_annotations branch 2 times, most recently from 86801ab to ec0e9ac Compare May 9, 2018 18:56
@swegner swegner changed the title Upgrade Byte Buddy and FindBugs annotations dependencies. Fix some compiler warnings in sdks/java/core May 9, 2018
@swegner
Copy link
Contributor Author

swegner commented May 9, 2018

R: @kennknowles

@kennknowles
Copy link
Member

Yes, please. My favorite compiler flag :-)

kennknowles
kennknowles approved these changes May 9, 2018
View reviewed changes
Copy link
Member

@kennknowles kennknowles left a comment

Choose a reason for hiding this comment

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

Just one readability nit.

sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/windowing/PaneInfo.java Outdated
@@ -129,14 +129,14 @@
}

private static byte encodedByte(boolean isFirst, boolean isLast, Timing timing) {
byte result = 0x0;
byte result = 0;
Copy link
Member

Choose a reason for hiding this comment

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

What was the warning here? TBH my preference would even be to use a binary literal 0b0 here but more importantly the lines below would be the very readable sequence making the logic obvious:

0b0001

0b0010

0b0100

Copy link
Contributor Author

Choose a reason for hiding this comment

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

NarrowingCompoundAssignment. The trouble is that Java doesn't have a way to specify a byte literal, so 0x1 is a treated as an integer and then triggers the ErrorProne warning on |= operator.

There's no reason we can't use 0b00 etc. Updated.

@swegner
Copy link
Contributor Author

swegner commented May 9, 2018

Java pre-commits are failing; seems that this change has somehow triggered the full wrath of ErrorProne: https://scans.gradle.com/s/whoprvzcdtcya/console-log?task=:beam-sdks-java-core:compileJava#L106

Not sure why these are being treated as failures, but let's see how much work it is to just fix the rest..

@swegner swegner force-pushed the upgrade_fb_annotations branch 5 times, most recently from 431e0d0 to c233139 Compare May 11, 2018 20:33
@swegner
Copy link
Contributor Author

swegner commented May 11, 2018

Ok, so I went and fixed all ErrorProne issues across SDK core such that we can enable -Werror. I'd love to see more of this across additional Java components.

@kennknowles PTAL

@kennknowles
Copy link
Member

Wow, that's amazing! I never thought we'd get there.

@swegner
Copy link
Contributor Author

swegner commented May 11, 2018

retest this please

@kennknowles
Copy link
Member

run java precommit

@kennknowles
Copy link
Member

The failure seems unlikely to be related, but curious anyhow.

@swegner swegner force-pushed the upgrade_fb_annotations branch 2 times, most recently from e2ccc08 to ddbaf49 Compare May 14, 2018 17:26
@swegner
Copy link
Contributor Author

swegner commented May 14, 2018

retest this please

1 similar comment
@swegner
Copy link
Contributor Author

swegner commented May 15, 2018

retest this please

@swegner
Copy link
Contributor Author

swegner commented May 15, 2018

Run Python PreCommit

@swegner
Copy link
Contributor Author

swegner commented May 15, 2018

All green! @kennknowles PTAL

@kennknowles
Copy link
Member

Great! Would you mind curating the commits now that it is g2g?

@swegner
Upgrade Byte Buddy and FindBugs annotations dependencies.
1b66e23 
This clears compiler warnings about SuppressFBWarnings
for dependent libaries using the latest version:

warning: Cannot find annotation method 'value()' in type
'SuppressWarnings': class file for
edu.umd.cs.findbugs.annotations.SuppressWarnings not found
@swegner
Fix ErrorProne warnings
ee76a22 
* http://errorprone.info/bugpattern/AssertEqualsArgumentOrderChecker
* http://errorprone.info/bugpattern/AssertionFailureIgnored
* http://errorprone.info/bugpattern/BoxedPrimitiveConstructor
* http://errorprone.info/bugpattern/CanonicalDuration
* http://errorprone.info/bugpattern/CatchFail
* http://errorprone.info/bugpattern/ClassCanBeStatic
* http://errorprone.info/bugpattern/ClassNewInstance
* http://errorprone.info/bugpattern/DefaultCharset
* http://errorprone.info/bugpattern/DoubleBraceInitialization
* http://errorprone.info/bugpattern/EqualsHashCode
* http://errorprone.info/bugpattern/EqualsIncompatibleType
* http://errorprone.info/bugpattern/FutureReturnValueIgnored
* http://errorprone.info/bugpattern/ImmutableEnumChecker
* http://errorprone.info/bugpattern/IntLongMath
* http://errorprone.info/bugpattern/JavaLangClash
* http://errorprone.info/bugpattern/JdkObsolete
* http://errorprone.info/bugpattern/MissingOverride
* http://errorprone.info/bugpattern/MutableConstantField
* http://errorprone.info/bugpattern/NarrowingCompoundAssignment
* http://errorprone.info/bugpattern/OperatorPrecedence
* http://errorprone.info/bugpattern/ReferenceEquality
* http://errorprone.info/bugpattern/ShortCircuitBoolean
* http://errorprone.info/bugpattern/StringSplitter
* http://errorprone.info/bugpattern/TypeParameterUnusedInFormals
* http://errorprone.info/bugpattern/UnsynchronizedOverridesSynchronized
@swegner
Remove isTriggerSpecified from WindowingStategy equality checks.
3f7ee68 
This field isn't retained on proto translation, and shouldn't be used
when checking for equality of WindowingStrategy instances.
@swegner
Copy link
Contributor Author

swegner commented May 16, 2018

Rebased with no other changes.

@swegner
Copy link
Contributor Author

swegner commented May 16, 2018

PTAL @kennknowles

@kennknowles kennknowles merged commit 518a29b into apache:master May 16, 2018
@swegner swegner deleted the upgrade_fb_annotations branch May 23, 2018 23:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kennknowles kennknowles kennknowles approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@swegner @kennknowles