Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

SuppressionCommentFilter does not suppress StrictDuplicateCode Warnings #26

Closed
romani opened this Issue · 8 comments

3 participants

@romani
Owner

Created: 2012-10-31
Creator: Christoph Böhme
SF issue: 687

The SuppressionCommentFilter does not suppress warnings generated by the StrictDuplicateCode module.

My checkstyle configuration is:

<module name="Checker">
  <module name="TreeWalker">
        <module name="FileContentsHolder"/>
        <module name="AvoidInlineConditionals"/>
  </module>
  <module name="StrictDuplicateCode">
    <property name="min" value="5"/>
  </module>
  <module name="SuppressionCommentFilter"/>
</module>

The AvoidInlineConditionals is only included to verify that suppression is configured correctly.

In the java file checkstyle is disabled using comments:

public class StrictDuplicateCodeTest {

        // CHECKSTYLE:OFF

        public static void main(String[] args) {
                int i = 1;
                i += i == 1 ? 2 : 3;
                /* Some random comments
                 * in order to
                 * produce some
                 * duplicate
                 * code.
                 */
                duplicatedCode(i);
        }

        public static int duplicatedCode(int j) {
                int i = 1;
                i += i == 1 ? 2 : 3;
                /* Some random comments
                 * in order to
                 * produce some
                 * duplicate
                 * code.
                 */
                i += j;
                return i;
        }

        // CHECKSTYLE:ON
}

In my understanding checkstyle should not produce any warnings when checking this file. However, when running checkstyle on the file I get the following output:

java -jar checkstyle-5.6/checkstyle-5.6-all.jar -c checkstyle.xml StrictDuplicateCodeTest.java
Starting audit...
StrictDuplicateCodeTest.java:6: 8 gleiche Zeilen in StrictDuplicateCodeTest.java, beginnend bei Zeile 18
Audit done.

If I remove the two CHECKSTYLE comments from the file then the output also includes two AvoidInlineConditional warnings:

java -jar checkstyle-5.6/checkstyle-5.6-all.jar -c checkstyle.xml StrictDuplicateCodeTest.java
Starting audit...
D:\\StrictDuplicateCodeTest.java:5:29: Der Bedingungsoperator sollte vermieden werden.
D:\\StrictDuplicateCodeTest.java:17:29: Der Bedingungsoperator sollte vermieden werden.
StrictDuplicateCodeTest.java:4: 8 gleiche Zeilen in StrictDuplicateCodeTest.java, beginnend bei Zeile 16
Audit done.
@ychulovskyy
Collaborator

I'll work on it.

@mkordas
Collaborator
@ychulovskyy
Collaborator

@mkordas Thanks for update!
@romani Could you please close the issue?

@romani
Owner

@ychulovskyy, here problem is not about a particular Check , you can substitude StrictDuplicateCheck to another Check that is not running under TreeWalker. Such Checks are called FileSet checks.

@ychulovskyy
Collaborator

Ok, I've found the issue.
Checker.process do the following:

        for (final FileSetCheck fsc : mFileSetChecks) {
            // They may also log!!!
            fsc.finishProcessing();
            fsc.destroy();
        }

Need to do the same but in separate loops. Otherwise destroy method will release file context and next check doesn't have it. As a result - filter.accept() will return true and we will see the audit message.
Will fix and create test in a few days.

@romani
Owner

@ychulovskyy , are you planing to provide fix or our team should do that ?

@ychulovskyy
Collaborator

@romani I'll provide till EOW. It's not so important since there is no more checks that do processing in finishProcessing() method. Only StrictDuplicateCodehad it.
So, need to think how to test the fix.

@ychulovskyy ychulovskyy referenced this issue from a commit in ychulovskyy/checkstyle
@ychulovskyy ychulovskyy Issue #26 SuppressionCommentFilter does not suppress StrictDuplicateC…
…ode Warnings
cadfbd8
@romani
Owner

that solution is accepted, but #820 is created to fix that conceptual problem.

fix is merged, will be in 6.5

@romani romani closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.