From 17043cf32fa56b026ab0f07be4250a978119eb51 Mon Sep 17 00:00:00 2001 From: Xavier Le Vourch Date: Tue, 14 Nov 2006 02:22:40 +0000 Subject: [PATCH] Fixed false positives in LocalVariableCouldBeFinal. git-svn-id: https://pmd.svn.sourceforge.net/svnroot/pmd/trunk@4791 51baf565-9d33-0410-a72c-fc3788e3496d --- pmd/etc/changelog.txt | 1 + .../LocalVariableCouldBeFinalTest.java | 13 +++++ .../AbstractOptimizationRule.java | 51 ++++++++++++------- 3 files changed, 48 insertions(+), 17 deletions(-) diff --git a/pmd/etc/changelog.txt b/pmd/etc/changelog.txt index 85c54ef3c0..89e34fe4db 100644 --- a/pmd/etc/changelog.txt +++ b/pmd/etc/changelog.txt @@ -33,6 +33,7 @@ Fixed several JDK 1.5 parsing bugs. Fixed several rules (exceptions on jdk 1.5 and jdk 1.6 source code). Fixed array handling in AvoidReassigningParameters and UnusedFormalParameter. Fixed bug in UselessOverridingMethod: false + when adding synchronization. +Fixed false positives in LocalVariableCouldBeFinal. Rules can now call RuleContext.getSourceType() if they need to make different checks on JDK 1.4 and 1.5 code. CloseResource rule now checks code without java.sql import. ArrayIsStoredDirectly rule now checks Constructors diff --git a/pmd/regress/test/net/sourceforge/pmd/rules/optimization/LocalVariableCouldBeFinalTest.java b/pmd/regress/test/net/sourceforge/pmd/rules/optimization/LocalVariableCouldBeFinalTest.java index 4bed4a9342..36ad719af3 100644 --- a/pmd/regress/test/net/sourceforge/pmd/rules/optimization/LocalVariableCouldBeFinalTest.java +++ b/pmd/regress/test/net/sourceforge/pmd/rules/optimization/LocalVariableCouldBeFinalTest.java @@ -35,6 +35,7 @@ public class LocalVariableCouldBeFinalTest extends SimpleAggregatorTst { new TestDescriptor(TEST7, "TEST7", 0, rule), new TestDescriptor(TEST8, "TEST8", 0, rule), new TestDescriptor(TEST9, "TEST9", 1, rule), + new TestDescriptor(TEST10, "TEST10", 0, rule), }); } @@ -116,4 +117,16 @@ public class LocalVariableCouldBeFinalTest extends SimpleAggregatorTst { " }" + PMD.EOL + "}"; + private static final String TEST10 = + "public class Foo {" + PMD.EOL + + " public void test1() {" + PMD.EOL + + " int a = 0;" + PMD.EOL + + " int b = 0;" + PMD.EOL + + " int c = 0;" + PMD.EOL + + " ++(a);" + PMD.EOL + + " --(b);" + PMD.EOL + + " (c)++;" + PMD.EOL + + " }" + PMD.EOL + + "}"; + } diff --git a/pmd/src/net/sourceforge/pmd/rules/optimization/AbstractOptimizationRule.java b/pmd/src/net/sourceforge/pmd/rules/optimization/AbstractOptimizationRule.java index 2bb8e9e18d..6671c3e709 100644 --- a/pmd/src/net/sourceforge/pmd/rules/optimization/AbstractOptimizationRule.java +++ b/pmd/src/net/sourceforge/pmd/rules/optimization/AbstractOptimizationRule.java @@ -5,22 +5,25 @@ */ package net.sourceforge.pmd.rules.optimization; +import java.util.Iterator; +import java.util.List; + import net.sourceforge.pmd.AbstractRule; import net.sourceforge.pmd.Rule; import net.sourceforge.pmd.ast.ASTAssignmentOperator; +import net.sourceforge.pmd.ast.ASTExpression; import net.sourceforge.pmd.ast.ASTLocalVariableDeclaration; import net.sourceforge.pmd.ast.ASTMethodDeclaration; import net.sourceforge.pmd.ast.ASTName; import net.sourceforge.pmd.ast.ASTPostfixExpression; import net.sourceforge.pmd.ast.ASTPreDecrementExpression; import net.sourceforge.pmd.ast.ASTPreIncrementExpression; +import net.sourceforge.pmd.ast.ASTPrimaryExpression; +import net.sourceforge.pmd.ast.ASTPrimaryPrefix; import net.sourceforge.pmd.ast.ASTVariableDeclaratorId; import net.sourceforge.pmd.ast.Node; import net.sourceforge.pmd.ast.SimpleNode; -import java.util.Iterator; -import java.util.List; - /** * Base class with utility methods for optimization rules * @@ -57,9 +60,7 @@ public class AbstractOptimizationRule extends AbstractRule implements Rule { List preinc = md.findChildrenOfType(ASTPreIncrementExpression.class); if (preinc != null && !preinc.isEmpty()) { for (Iterator it = preinc.iterator(); it.hasNext();) { - ASTPreIncrementExpression ie = (ASTPreIncrementExpression) it.next(); - Node prefix = ie.jjtGetChild(0).jjtGetChild(0); - if (prefix.jjtGetNumChildren() > 0 && ((ASTName) prefix.jjtGetChild(0)).hasImageEqualTo(varName)) { + if (isName((Node)it.next(), varName)) { return true; } } @@ -69,9 +70,7 @@ public class AbstractOptimizationRule extends AbstractRule implements Rule { List predec = md.findChildrenOfType(ASTPreDecrementExpression.class); if (predec != null && !predec.isEmpty()) { for (Iterator it = predec.iterator(); it.hasNext();) { - ASTPreDecrementExpression de = (ASTPreDecrementExpression) it.next(); - Node prefix = de.jjtGetChild(0).jjtGetChild(0); - if (prefix.jjtGetNumChildren() > 0 && ((ASTName) prefix.jjtGetChild(0)).hasImageEqualTo(varName)) { + if (isName((Node)it.next(), varName)) { return true; } } @@ -81,15 +80,8 @@ public class AbstractOptimizationRule extends AbstractRule implements Rule { if (pf != null && !pf.isEmpty()) { for (Iterator it = pf.iterator(); it.hasNext();) { ASTPostfixExpression pe = (ASTPostfixExpression) it.next(); - if ((pe.hasImageEqualTo("++") || pe.hasImageEqualTo("--"))) { - SimpleNode first = (SimpleNode) pe.jjtGetChild(0); - SimpleNode second = (SimpleNode) first.jjtGetChild(0); - if (second.jjtGetNumChildren() == 0 || !(second.jjtGetChild(0) instanceof ASTName)) { - continue; - } - ASTName name = (ASTName) second.jjtGetChild(0); - if (name.hasImageEqualTo(varName)) { + if (isName(pe, varName)) { return true; } } @@ -98,6 +90,31 @@ public class AbstractOptimizationRule extends AbstractRule implements Rule { return false; } + private final boolean isName(Node node, String varName) { + Node first = node.jjtGetChild(0); + Node second = first.jjtGetChild(0); + if (second.jjtGetNumChildren() == 0) { + return false; + } + + Node third = second.jjtGetChild(0); + if (!(third instanceof ASTName)) { + if (first instanceof ASTPrimaryExpression) { + // deals with extra parenthesis: + // "(varName)++" instead of "varName++" for instance + if (first.jjtGetNumChildren() != 1) { + return false; + } + if (second instanceof ASTPrimaryPrefix && second.jjtGetNumChildren() == 1 && + third instanceof ASTExpression && third.jjtGetNumChildren() == 1) { + return isName(third, varName); + } + } + return false; + } + ASTName name = (ASTName) third; + return name.hasImageEqualTo(varName); + } private final boolean variableAssigned(final String varName, final List assignments) { if (assignments == null || assignments.isEmpty()) {