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

AVRO-1047: Java: Fix remaining javac warnings. #250

Closed
wants to merge 1 commit into from

Conversation

mkw
Copy link
Contributor

@mkw mkw commented Sep 27, 2017

AVRO-1913 fixed most warnings, but missed a few unchecked usages and left in some redundant casts. This patch eliminates remaining warnings even with the -Xlint:all flag added to javac. With no more warnings to surpress, the @SuppressWarnings("all") annotation isremoved from the generated code, as well.

@nielsbasjes
Copy link
Contributor

First impression of this change is good.
Things that need attention:

  1. Can you check if it is easily possible to force the regular build to compile the generated code with this -Xlint:all ? That way we can spot new problems in this area much faster.
  2. When I run the full build (i.e. mvn clean package) I get test errors that the newly generated classes do not match the versions in the code base. (Position.java and Player.java) This is a normal effect of changing the code generation.
  3. This improvement is not mentioned yet in the CHANGES.txt

AVRO-1913 fixed most warnings, but missed a few unchecked usages and
left in some redundant casts.  This patch eliminates remaining
warnings even with the -Xlint:all flag added to javac.  With no more
warnings to surpress, the @SuppressWarnings("all") annotation is
removed from the generated code, as well.  The TestSpecificCompiler
test has been modified to pass -Xlint:all to the compiler and assert
that no warnings are present in compiled code.
@mkw
Copy link
Contributor Author

mkw commented Dec 12, 2017

I modified org.apache.avro.compiler.specific.TestSpecificCompiler#assertCompilesWithJavaCompiler to catch these in tests:

  1. Passed -Xlint:all to the compile task.
  2. Added a compiler diagnostic listener to collect warnings and an assert that fails the test when present.
  3. Added a test schema that triggers the warnings that were fixed by the original commit (union&fixed types).

Also fixed the failing tests, added a summary in CHANGES.txt, rebased off of branch-1.8, and squashed the history into a single commit. Let me know if I should do anything else!

@nielsbasjes
Copy link
Contributor

+1
Thanks for this cleanup!

@nielsbasjes
Copy link
Contributor

Committed to both branch-1.8 and master.
Thanks.

(Can you please close this pull request? Thanks.)

@mkw mkw closed this Dec 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants