From 73ebf2603a925f5c8a8611cbd75cf6acc1a338cc Mon Sep 17 00:00:00 2001 From: Romain Pelisse Date: Sat, 15 Aug 2009 16:40:53 +0000 Subject: [PATCH] Fixing bug 2826119 - False +: DoubleCheckedLocking warning with volatile field git-svn-id: https://pmd.svn.sourceforge.net/svnroot/pmd/branches/pmd/4.2.x@6978 51baf565-9d33-0410-a72c-fc3788e3496d --- pmd/etc/changelog.txt | 2 ++ .../rules/basic/xml/DoubleCheckedLocking.xml | 30 ++++++++++++++--- .../pmd/rules/DoubleCheckedLocking.java | 33 ++++++++++++++++--- 3 files changed, 57 insertions(+), 8 deletions(-) diff --git a/pmd/etc/changelog.txt b/pmd/etc/changelog.txt index 8f21b9705e..8265f8d053 100644 --- a/pmd/etc/changelog.txt +++ b/pmd/etc/changelog.txt @@ -6,6 +6,8 @@ Fixed bug 2606609 - False "UnusedImports" positive in package-info.java Fixed bug 2645268 - ClassCastException in UselessOperationOnImmutable.getDeclaration Fixed bug 2724653 - AvoidThreadGroup reports false positives Fixed bug 2835074 - False -: DoubleCheckedLocking with reversed null check +Fixed bug 2826119 - False +: DoubleCheckedLocking warning with volatile field + Correct -benchmark reporting of Rule visits via the RuleChain Fix issue with Type Resolution incorrectly handling of Classes with same name as a java.lang Class. diff --git a/pmd/regress/test/net/sourceforge/pmd/rules/basic/xml/DoubleCheckedLocking.xml b/pmd/regress/test/net/sourceforge/pmd/rules/basic/xml/DoubleCheckedLocking.xml index 38f9ebbeed..f8ac424c58 100644 --- a/pmd/regress/test/net/sourceforge/pmd/rules/basic/xml/DoubleCheckedLocking.xml +++ b/pmd/regress/test/net/sourceforge/pmd/rules/basic/xml/DoubleCheckedLocking.xml @@ -57,8 +57,8 @@ public class Foo { 1 1 + + + 0 + + diff --git a/pmd/src/net/sourceforge/pmd/rules/DoubleCheckedLocking.java b/pmd/src/net/sourceforge/pmd/rules/DoubleCheckedLocking.java index 747ddb8371..9af1a732ba 100644 --- a/pmd/src/net/sourceforge/pmd/rules/DoubleCheckedLocking.java +++ b/pmd/src/net/sourceforge/pmd/rules/DoubleCheckedLocking.java @@ -3,9 +3,14 @@ */ package net.sourceforge.pmd.rules; +import java.util.ArrayList; +import java.util.List; + import net.sourceforge.pmd.AbstractJavaRule; import net.sourceforge.pmd.ast.ASTAssignmentOperator; import net.sourceforge.pmd.ast.ASTClassOrInterfaceDeclaration; +import net.sourceforge.pmd.ast.ASTCompilationUnit; +import net.sourceforge.pmd.ast.ASTFieldDeclaration; import net.sourceforge.pmd.ast.ASTIfStatement; import net.sourceforge.pmd.ast.ASTLiteral; import net.sourceforge.pmd.ast.ASTMethodDeclaration; @@ -18,11 +23,9 @@ import net.sourceforge.pmd.ast.ASTReturnStatement; import net.sourceforge.pmd.ast.ASTStatementExpression; import net.sourceforge.pmd.ast.ASTSynchronizedStatement; import net.sourceforge.pmd.ast.ASTType; +import net.sourceforge.pmd.ast.ASTVariableDeclaratorId; import net.sourceforge.pmd.ast.Node; -import java.util.ArrayList; -import java.util.List; - /** * void method() { * if(x == null) { @@ -44,12 +47,33 @@ import java.util.List; */ public class DoubleCheckedLocking extends AbstractJavaRule { + List volatileFields; + + public Object visit(ASTCompilationUnit compilationUnit, Object data) { + if ( volatileFields == null ) { + volatileFields = new ArrayList(0); + } else { + volatileFields.clear(); + } + return super.visit(compilationUnit, data); + } + public Object visit(ASTClassOrInterfaceDeclaration node, Object data) { if (node.isInterface()) { return data; } return super.visit(node, data); } + + public Object visit(ASTFieldDeclaration fieldDeclaration, Object data) { + if ( fieldDeclaration.isVolatile() ) { + List declarators = fieldDeclaration.findChildrenOfType(ASTVariableDeclaratorId.class); + for (ASTVariableDeclaratorId declarator : declarators ) { + this.volatileFields.add(declarator.getImage()); + } + } + return super.visit(fieldDeclaration, data); + } public Object visit(ASTMethodDeclaration node, Object data) { if (node.getResultType().isVoid()) { @@ -76,7 +100,8 @@ public class DoubleCheckedLocking extends AbstractJavaRule { if (lastChild instanceof ASTPrimaryPrefix) { returnVariableName = getNameFromPrimaryPrefix((ASTPrimaryPrefix) lastChild); } - if (returnVariableName == null) { + // With Java5 and volatile keyword, DCL is no longer an issue + if (returnVariableName == null || this.volatileFields.contains(returnVariableName)) { return super.visit(node, data); } List isl = new ArrayList();