From 4712cb1e66e212845971fa69f49c14d970100734 Mon Sep 17 00:00:00 2001 From: Sergey Prigogin Date: Sun, 17 Jul 2011 17:20:16 -0700 Subject: [PATCH] Bug 315528 - [fp] Non-virtual destructor diagnostics doesn't take superclass into account. Fix and additional tests including the ones contributed by Tomasz Wesolowski. --- .../checkers/NonVirtualDestructor.java | 230 +++++------------- .../NonVirtualDestructorCheckerTest.java | 59 ++++- 2 files changed, 122 insertions(+), 167 deletions(-) diff --git a/codan/org.eclipse.cdt.codan.checkers/src/org/eclipse/cdt/codan/internal/checkers/NonVirtualDestructor.java b/codan/org.eclipse.cdt.codan.checkers/src/org/eclipse/cdt/codan/internal/checkers/NonVirtualDestructor.java index 076e2e1878c..f4e365cd037 100644 --- a/codan/org.eclipse.cdt.codan.checkers/src/org/eclipse/cdt/codan/internal/checkers/NonVirtualDestructor.java +++ b/codan/org.eclipse.cdt.codan.checkers/src/org/eclipse/cdt/codan/internal/checkers/NonVirtualDestructor.java @@ -11,10 +11,8 @@ *******************************************************************************/ package org.eclipse.cdt.codan.internal.checkers; -import org.eclipse.cdt.codan.checkers.CodanCheckersActivator; import org.eclipse.cdt.codan.core.cxx.model.AbstractIndexAstChecker; import org.eclipse.cdt.core.dom.ast.ASTVisitor; -import org.eclipse.cdt.core.dom.ast.DOMException; import org.eclipse.cdt.core.dom.ast.IASTDeclSpecifier; import org.eclipse.cdt.core.dom.ast.IASTName; import org.eclipse.cdt.core.dom.ast.IASTNode; @@ -33,183 +31,89 @@ import org.eclipse.cdt.internal.core.dom.parser.cpp.ICPPInternalBinding; * @author Alena Laskavaia */ public class NonVirtualDestructor extends AbstractIndexAstChecker { - public static final String ER_ID = "org.eclipse.cdt.codan.internal.checkers.NonVirtualDestructorProblem"; //$NON-NLS-1$ + public static final String PROBLEM_ID = "org.eclipse.cdt.codan.internal.checkers.NonVirtualDestructorProblem"; //$NON-NLS-1$ public void processAst(IASTTranslationUnit ast) { // Traverse the AST using the visitor pattern. ast.accept(new OnEachClass()); } - class OnEachClass extends ASTVisitor { - private IASTName className; - private IBinding virtualMethod; - private IBinding destructor; - private IASTDeclSpecifier warningLocation; + private static ICPPMethod getDestructor(ICPPClassType classType) { + for (ICPPMethod method : classType.getDeclaredMethods()) { + if (method.isDestructor()) { + return method; + } + } + return null; + } + private static boolean hasVirtualDestructor(ICPPClassType classType) { + ICPPMethod destructor = getDestructor(classType); + if (destructor != null && destructor.isVirtual()) { + return true; + } + ICPPBase[] bases = classType.getBases(); + for (ICPPBase base : bases) { + IBinding baseClass = base.getBaseClass(); + if (baseClass instanceof ICPPClassType) { + if (hasVirtualDestructor((ICPPClassType) baseClass)) { + return true; + } + } + } + return false; + } + + private class OnEachClass extends ASTVisitor { OnEachClass() { shouldVisitDeclSpecifiers = true; } public int visit(IASTDeclSpecifier decl) { - if (isClassDecl(decl)) { - try { - boolean err = hasErrorCondition(decl); - if (err) { - String clazz = className.toString(); - String method = virtualMethod.getName(); - IASTNode node = decl; - if (destructor != null) { - if (destructor instanceof ICPPInternalBinding) { - IASTNode[] decls = ((ICPPInternalBinding) destructor).getDeclarations(); - if (warningLocation == null) { - if (decls != null && decls.length > 0) - node = decls[0]; - } else { - node = warningLocation; - } - } - } - reportProblem(ER_ID, node, clazz, method); - } - } catch (DOMException e) { - // Ignore, not an error. - } catch (Exception e) { - CodanCheckersActivator.log(e); + if (decl instanceof ICPPASTCompositeTypeSpecifier) { + ICPPASTCompositeTypeSpecifier spec = (ICPPASTCompositeTypeSpecifier) decl; + IASTName className = spec.getName(); + IBinding binding = className.resolveBinding(); + if (!(binding instanceof ICPPClassType)) { + return PROCESS_SKIP; } + ICPPClassType classType = (ICPPClassType) binding; + if (hasVirtualDestructor(classType)) { + return PROCESS_SKIP; + } + ICPPMethod virtualMethod = null; + for (ICPPMethod method : classType.getAllDeclaredMethods()) { + if (method.isPureVirtual()) { + // Class has at least one pure virtual method, it is abstract + // and cannot be instantiated. + return PROCESS_SKIP; + } else if (method.isVirtual() && !method.isDestructor()) { + virtualMethod = method; + } + } + if (virtualMethod == null) { + return PROCESS_SKIP; + } + ICPPMethod destructor = getDestructor(classType); + if (destructor != null && + destructor.getVisibility() != ICPPASTVisibilityLabel.v_public && + classType.getFriends().length == 0) { + // No error if the destructor is protected or private and there are no friends. + return PROCESS_SKIP; + } + + IASTNode node = decl; + if (destructor instanceof ICPPInternalBinding) { + IASTNode[] decls = ((ICPPInternalBinding) destructor).getDeclarations(); + if (decls != null && decls.length > 0) { + node = decls[0]; + } + } + reportProblem(PROBLEM_ID, node, className.getSimpleID().toString(), + virtualMethod.getName()); return PROCESS_SKIP; } return PROCESS_CONTINUE; } - - /** - * @param decl - * @throws DOMException - */ - private boolean hasErrorCondition(IASTDeclSpecifier decl) throws DOMException { - ICPPASTCompositeTypeSpecifier spec = (ICPPASTCompositeTypeSpecifier) decl; - className = spec.getName(); - IBinding binding = className.resolveBinding(); - if ((binding instanceof ICPPClassType) == false) { - return false; - } - ICPPClassType classType = (ICPPClassType) binding; - virtualMethod = null; - destructor = null; - warningLocation = null; - // Check for the following conditions: - // class has own virtual method and own non-virtual destructor - // class has own virtual method and base non-virtual destructor - // class has base virtual method and own non-virtual destructor - ICPPMethod[] declaredMethods = classType.getDeclaredMethods(); - boolean hasOwnVirtualMethod = false; - boolean hasOwnNonVirtualDestructor = false; // implicit destructor - boolean hasDestructor = false; - boolean hasVirtualMethod = false; - for (ICPPMethod method : declaredMethods) { - if (method.isPureVirtual()) { - // Class has at least one pure virtual method, it is abstract and cannot be instantiated. - return false; - } else if (method.isVirtual() && !method.isDestructor()) { - hasOwnVirtualMethod = true; - virtualMethod = method; - } - if (method.isDestructor()) { - hasDestructor = true; - if (!method.isVirtual()) { - hasOwnNonVirtualDestructor = true; - destructor = method; - } - } - } - boolean hasVirtualDestructor = false; - // Class has own virtual method and own non-virtual destructor. - if (hasOwnVirtualMethod && hasOwnNonVirtualDestructor) { - if (classType.getFriends().length == 0) { - if (destructor instanceof ICPPMethod) { - // Check visibility of dtor. No error if it is protected or private. - if (((ICPPMethod) destructor).getVisibility() != ICPPASTVisibilityLabel.v_public) { - return false; - } - } - // Check if one of its base classes has a virtual destructor. - return !hasVirtualDtorInBaseClass(classType); - } - // Destructor can be called from a class declared as friend. - return true; - } - // Class has virtual methods and implicit public destructor. - if (hasOwnVirtualMethod && !hasDestructor && classType.getBases().length == 0) { - return true; - } - // Class does not have virtual methods but has virtual destructor - // - not an error - if (!hasOwnVirtualMethod && hasDestructor && !hasOwnNonVirtualDestructor) { - return false; - } - // Check inherited methods - ICPPMethod[] allDeclaredMethods = classType.getAllDeclaredMethods(); - for (ICPPMethod method : allDeclaredMethods) { - if (method.isVirtual() && !method.isDestructor()) { - hasVirtualMethod = true; - if (virtualMethod == null) - virtualMethod = method; - } - if (method.isDestructor()) { - hasDestructor = true; - if (method.isVirtual()) { - hasVirtualDestructor = true; - } else if (destructor == null) { - destructor = method; - } - } - } - if (hasOwnVirtualMethod) { - // Class has own virtual method and base non-virtual destructor. - if (hasDestructor && !hasVirtualDestructor) { - boolean noVirtualDtorInBase = !hasVirtualDtorInBaseClass(classType); - if (noVirtualDtorInBase) { - warningLocation = decl; - } - return noVirtualDtorInBase; - } - } else if (hasVirtualMethod) { - // Class has base virtual method and own non-virtual destructor. - if (hasOwnNonVirtualDestructor) { - return true; - } - boolean noVirtualDtorInBase = !hasVirtualDtorInBaseClass(classType); - if (noVirtualDtorInBase) { - warningLocation = decl; - } - return noVirtualDtorInBase; - } - return false; - } - - private boolean hasVirtualDtorInBaseClass(ICPPClassType classType) { - ICPPBase[] bases = classType.getBases(); - for (ICPPBase base : bases) { - if (!(base.getBaseClass() instanceof ICPPClassType)) { - continue; - } - ICPPClassType testedBaseClass = (ICPPClassType) base.getBaseClass(); - ICPPMethod[] declaredBaseMethods = testedBaseClass.getDeclaredMethods(); - for (ICPPMethod method : declaredBaseMethods) { - if (method.isPureVirtual()) { - return false; - } - if (method.isDestructor() && method.isVirtual()) { - return true; - } - } - if (hasVirtualDtorInBaseClass(testedBaseClass)) - return true; - } - return false; - } - - private boolean isClassDecl(IASTDeclSpecifier decl) { - return decl instanceof ICPPASTCompositeTypeSpecifier; - } } } diff --git a/codan/org.eclipse.cdt.codan.core.test/src/org/eclipse/cdt/codan/core/internal/checkers/NonVirtualDestructorCheckerTest.java b/codan/org.eclipse.cdt.codan.core.test/src/org/eclipse/cdt/codan/core/internal/checkers/NonVirtualDestructorCheckerTest.java index a86b87dafb2..4a587027b88 100644 --- a/codan/org.eclipse.cdt.codan.core.test/src/org/eclipse/cdt/codan/core/internal/checkers/NonVirtualDestructorCheckerTest.java +++ b/codan/org.eclipse.cdt.codan.core.test/src/org/eclipse/cdt/codan/core/internal/checkers/NonVirtualDestructorCheckerTest.java @@ -14,7 +14,7 @@ import org.eclipse.cdt.codan.core.test.CheckerTestCase; import org.eclipse.cdt.codan.internal.checkers.NonVirtualDestructor; /** - * Test for {@see NonVirtualDestructor} class. + * Test for {@link NonVirtualDestructor} class. */ public class NonVirtualDestructorCheckerTest extends CheckerTestCase { @Override @@ -25,7 +25,7 @@ public class NonVirtualDestructorCheckerTest extends CheckerTestCase { @Override public void setUp() throws Exception { super.setUp(); - enableProblems(NonVirtualDestructor.ER_ID); + enableProblems(NonVirtualDestructor.PROBLEM_ID); } // struct A { @@ -86,7 +86,7 @@ public class NonVirtualDestructorCheckerTest extends CheckerTestCase { // struct B { // ~B(); // ok. // }; - public void testVirtualDtorInBaseClass() { + public void testVirtualDtorInBaseClass1() { loadCodeAndRun(getAboveComment()); checkNoErrors(); } @@ -108,6 +108,22 @@ public class NonVirtualDestructorCheckerTest extends CheckerTestCase { checkNoErrors(); } + // class A { + // public: + // virtual ~A(); + // }; + // + // class B : public A { + // public: + // ~B(); + // virtual void m(); + // friend class C; + // }; + public void testVirtualDtorInBaseClass3() { + loadCodeAndRun(getAboveComment()); + checkNoErrors(); + } + // struct A { // virtual void f() { }; // ~A(); // warn! public non-virtual dtor. @@ -122,7 +138,7 @@ public class NonVirtualDestructorCheckerTest extends CheckerTestCase { // // struct E : public D { // }; - public void testNonVirtualDtorInBaseClass2() { + public void testNonVirtualDtorInBaseClass() { loadCodeAndRun(getAboveComment()); checkErrorLines(3, 7, 11, 13); } @@ -143,4 +159,39 @@ public class NonVirtualDestructorCheckerTest extends CheckerTestCase { loadCodeAndRun(getAboveComment()); checkNoErrors(); } + + // struct Base { + // }; + // struct Derived : Base { + // virtual void bar(); + // }; + public void testImplicitDtorInBaseClass() { + loadCodeAndRun(getAboveComment()); + checkErrorLines(3); + } + + // struct Base { + // ~Base(); + // }; + // struct Derived : Base { + // virtual void bar(); + // }; + public void testExplicitDtorInBaseClass() { + loadCodeAndRun(getAboveComment()); + checkErrorLines(4); + } + + // struct IBase { + // virtual void foo() = 0; + // }; + // struct IDerived : IBase { + // }; + // struct ADerived : IDerived { + // void foo(); + // virtual void bar() = 0; + // }; + public void testInheritedAbstractClasses() { + loadCodeAndRun(getAboveComment()); + checkNoErrors(); + } }