Avro
  1. Avro
  2. AVRO-82

A large number of Java warnings due to unused vairables etc.

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.1.0
    • Component/s: java
    • Labels:
      None

      Description

      I', creating this JIRA to submit a patch that fix the following:

      • Unused imports
      • Unused private fields, local variables
      • Use of generic types without parameters
      • Missing setialVersionUID for serializable classes

      There are more than 50 compiler warnings due to these.

      1. AVRO-82.patch
        23 kB
        Thiruvalluvan M. G.
      2. AVRO-82.patch
        37 kB
        Thiruvalluvan M. G.

        Activity

        Hide
        Doug Cutting added a comment -

        These are warnings from your IDE, not warnings seen when building with Ant, right? Compiling with Ant generates no warnings for me. Perhaps we should add Checkstyle and/or FindBugs to the Ant compilation if we wish to raise the warning bar?

        Also some of the unused fields are fields that should perhaps remain. For example, in DataFileReader the bug is that we're missing a 'getCount()' accessor, not that we have an unused field. And in Requestor and Responder, those local variables will soon be used by AVRO-66, a patch we don't want to generate spurious conflicts with.

        Removing unused imports seems fine, but rearranging imports can break other patches. Also, replacing a wildcard import with a non-wildcarded long-term makes future unused imports more likely for folks like me who do not use an IDE (unless we add an equivalent Checkstyle test). So, again, I'm hesitant to encourage periodic cleanups of this sort without automatic testing that's common to all developers.

        If we add Checkstyle and/or FindBugs to Ant compilation, to be effective, we should fail tests if there are any warnings. Both support the suppression of warnings for specific files, if needed.

        Show
        Doug Cutting added a comment - These are warnings from your IDE, not warnings seen when building with Ant, right? Compiling with Ant generates no warnings for me. Perhaps we should add Checkstyle and/or FindBugs to the Ant compilation if we wish to raise the warning bar? Also some of the unused fields are fields that should perhaps remain. For example, in DataFileReader the bug is that we're missing a 'getCount()' accessor, not that we have an unused field. And in Requestor and Responder, those local variables will soon be used by AVRO-66 , a patch we don't want to generate spurious conflicts with. Removing unused imports seems fine, but rearranging imports can break other patches. Also, replacing a wildcard import with a non-wildcarded long-term makes future unused imports more likely for folks like me who do not use an IDE (unless we add an equivalent Checkstyle test). So, again, I'm hesitant to encourage periodic cleanups of this sort without automatic testing that's common to all developers. If we add Checkstyle and/or FindBugs to Ant compilation, to be effective, we should fail tests if there are any warnings. Both support the suppression of warnings for specific files, if needed.
        Hide
        Doug Cutting added a comment -

        Let's add Checkstyle to the build before we commit something like this, so the changes are maintainable by all.

        Show
        Doug Cutting added a comment - Let's add Checkstyle to the build before we commit something like this, so the changes are maintainable by all.
        Hide
        Scott Carey added a comment -

        Or, I believe you can just change the options that Ant is passing to javac, and add -Xlint. I'm not sure if that addresses all of the above, but it does for 3 of the four for sure.

        http://java.sun.com/javase/6/docs/technotes/tools/solaris/javac.html

        Show
        Scott Carey added a comment - Or, I believe you can just change the options that Ant is passing to javac, and add -Xlint. I'm not sure if that addresses all of the above, but it does for 3 of the four for sure. http://java.sun.com/javase/6/docs/technotes/tools/solaris/javac.html
        Hide
        Doug Cutting added a comment -

        > you can just change the options that Ant is passing to javac, and add -Xlint

        In place of the 50 warnings addressed above, this generates 20 warnings. The intersection between the sets is around 15, i.e., -Xlint generates warnings that Thiru's IDE does not, several of which seem spurious to me (e.g., a 'fallthrough' warning).

        We need to agree on a single standard, one that's integrated into the Ant build.

        Show
        Doug Cutting added a comment - > you can just change the options that Ant is passing to javac, and add -Xlint In place of the 50 warnings addressed above, this generates 20 warnings. The intersection between the sets is around 15, i.e., -Xlint generates warnings that Thiru's IDE does not, several of which seem spurious to me (e.g., a 'fallthrough' warning). We need to agree on a single standard, one that's integrated into the Ant build.
        Hide
        Thiruvalluvan M. G. added a comment -

        I attempted to include checkstyle into ant builds. It can catch some more problems that are not caught by -Xlint or eclipse. But it left several problems uncaught. Speficifically, it does not catch unused local variables and missing parameters to genetic types. In some cases it does not catch all the unused imports.

        I've attached the patch that sets up checkstyle. I have included the following checks to start with. We can add more tests as we move along.

        The following checks found some violations

        • Tab characters in java files instead of spaces
        • Absence of a blank line at the end of the file
        • Wildcard imports
        • Redundant imports
        • Unused imports
        • Redundant modifiers (such as "public" for methods in interface)
        • Redundant throws (throw clause with an exception and its subclass for the same method or throw clause for a subclass of RuntimeException)
        • Array type style (use "boolean[] b" instead of "boolean b[]", for example)

        The following check didn't find any

        • Naming conventions for constants
        • Empty statements
        • Illegal instantiations (where factory should be used)
        • Simplify overly complex boolean expressions
        • Interface is type (Bloch Effective Java item 17)
        • Upper case L is used in long integer constants

        The main reason for me create this JIRA is to avoid the large number of warnings I see in Eclipse. I'm forced to remember the number of warnings that the "clean" copy from svn generates so that I can ensure I don't introduce any more. Since we don't seem to have any simple alternative tool to catch all the warnings that eclipse generates, I guess have to just continue what I do.

        Show
        Thiruvalluvan M. G. added a comment - I attempted to include checkstyle into ant builds. It can catch some more problems that are not caught by -Xlint or eclipse. But it left several problems uncaught. Speficifically, it does not catch unused local variables and missing parameters to genetic types. In some cases it does not catch all the unused imports. I've attached the patch that sets up checkstyle. I have included the following checks to start with. We can add more tests as we move along. The following checks found some violations Tab characters in java files instead of spaces Absence of a blank line at the end of the file Wildcard imports Redundant imports Unused imports Redundant modifiers (such as "public" for methods in interface) Redundant throws (throw clause with an exception and its subclass for the same method or throw clause for a subclass of RuntimeException) Array type style (use "boolean[] b" instead of "boolean b[]", for example) The following check didn't find any Naming conventions for constants Empty statements Illegal instantiations (where factory should be used) Simplify overly complex boolean expressions Interface is type (Bloch Effective Java item 17) Upper case L is used in long integer constants The main reason for me create this JIRA is to avoid the large number of warnings I see in Eclipse. I'm forced to remember the number of warnings that the "clean" copy from svn generates so that I can ensure I don't introduce any more. Since we don't seem to have any simple alternative tool to catch all the warnings that eclipse generates, I guess have to just continue what I do.
        Hide
        Doug Cutting added a comment -

        I just committed this with a few changes:

        • added a few more checks;
        • made it so that so unit tests will not pass if checkstyle fails;
        • since checkstyle is LGPL, we cannot bundle it. I improved our use of Ivy so that code that's only required to run tests is not bundled. This also improves dependencies in our generated Maven POM file.

        > The main reason for me create this JIRA is to avoid the large number of warnings I see in Eclipse.

        Can you change your Eclipse settings, so that its warnings match those of checkstyle? Perhaps we could even include an appropriate Eclipse settings file with the project.

        Thanks, Thiru!

        Show
        Doug Cutting added a comment - I just committed this with a few changes: added a few more checks; made it so that so unit tests will not pass if checkstyle fails; since checkstyle is LGPL, we cannot bundle it. I improved our use of Ivy so that code that's only required to run tests is not bundled. This also improves dependencies in our generated Maven POM file. > The main reason for me create this JIRA is to avoid the large number of warnings I see in Eclipse. Can you change your Eclipse settings, so that its warnings match those of checkstyle? Perhaps we could even include an appropriate Eclipse settings file with the project. Thanks, Thiru!

          People

          • Assignee:
            Thiruvalluvan M. G.
            Reporter:
            Thiruvalluvan M. G.
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development