diff options
2 files changed, 44 insertions, 31 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 ead3e381331..076e2e1878c 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 @@ -36,7 +36,7 @@ public class NonVirtualDestructor extends AbstractIndexAstChecker { public static final String ER_ID = "org.eclipse.cdt.codan.internal.checkers.NonVirtualDestructorProblem"; //$NON-NLS-1$ public void processAst(IASTTranslationUnit ast) { - // Traverse the ast using the visitor pattern. + // Traverse the AST using the visitor pattern. ast.accept(new OnEachClass()); } @@ -102,25 +102,28 @@ public class NonVirtualDestructor extends AbstractIndexAstChecker { // class has base virtual method and own non-virtual destructor ICPPMethod[] declaredMethods = classType.getDeclaredMethods(); boolean hasOwnVirtualMethod = false; - boolean hasOwnNonVirDestructor = false; // implicit destructor + boolean hasOwnNonVirtualDestructor = false; // implicit destructor boolean hasDestructor = false; boolean hasVirtualMethod = false; for (ICPPMethod method : declaredMethods) { - if (method.isVirtual() && !method.isDestructor()) { + 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()) { - hasOwnNonVirDestructor = true; + hasOwnNonVirtualDestructor = true; destructor = method; } } } boolean hasVirtualDestructor = false; // Class has own virtual method and own non-virtual destructor. - if (hasOwnVirtualMethod && hasOwnNonVirDestructor) { + if (hasOwnVirtualMethod && hasOwnNonVirtualDestructor) { if (classType.getFriends().length == 0) { if (destructor instanceof ICPPMethod) { // Check visibility of dtor. No error if it is protected or private. @@ -134,15 +137,13 @@ public class NonVirtualDestructor extends AbstractIndexAstChecker { // Destructor can be called from a class declared as friend. return true; } - // Class does have virtual methods and no destructors, compiler will generate implicit public destructor - if (hasOwnVirtualMethod && !hasDestructor) { - if (classType.getBases().length == 0) { - 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 && !hasOwnNonVirDestructor) { + if (!hasOwnVirtualMethod && hasDestructor && !hasOwnNonVirtualDestructor) { return false; } // Check inherited methods @@ -157,9 +158,8 @@ public class NonVirtualDestructor extends AbstractIndexAstChecker { hasDestructor = true; if (method.isVirtual()) { hasVirtualDestructor = true; - } else { - if (destructor == null) - destructor = method; + } else if (destructor == null) { + destructor = method; } } } @@ -172,10 +172,9 @@ public class NonVirtualDestructor extends AbstractIndexAstChecker { } return noVirtualDtorInBase; } - } else if (hasVirtualMethod) { // Class has base virtual method and own non-virtual destructor. - if (hasOwnNonVirDestructor) { + if (hasOwnNonVirtualDestructor) { return true; } boolean noVirtualDtorInBase = !hasVirtualDtorInBaseClass(classType); @@ -196,6 +195,9 @@ public class NonVirtualDestructor extends AbstractIndexAstChecker { 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; } @@ -206,15 +208,8 @@ public class NonVirtualDestructor extends AbstractIndexAstChecker { return false; } - /** - * @param decl - * @return - */ private boolean isClassDecl(IASTDeclSpecifier decl) { - if (decl instanceof ICPPASTCompositeTypeSpecifier) { - return true; - } - return false; + 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 eaefcc8cc36..a86b87dafb2 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 @@ -48,7 +48,7 @@ public class NonVirtualDestructorCheckerTest extends CheckerTestCase { } // struct A { - // virtual void f() = 0; + // virtual void f() { }; // ~A(); // warn! public non-virtual dtor. // }; public void testPublicVirtualDtorInClass() { @@ -57,7 +57,7 @@ public class NonVirtualDestructorCheckerTest extends CheckerTestCase { } // struct A { - // virtual void f() = 0; + // virtual void f() { }; // // warn! implicit public non-virtual dtor. // }; public void testImplicitPublicNonVirtualDtorInClass() { @@ -68,7 +68,7 @@ public class NonVirtualDestructorCheckerTest extends CheckerTestCase { // struct F { }; // // struct A { - // virtual void f() = 0; + // virtual void f() { }; // private: // friend class F; // ~A(); // warn! can be called from class F. @@ -79,7 +79,7 @@ public class NonVirtualDestructorCheckerTest extends CheckerTestCase { } // struct A { - // virtual void f() = 0; + // virtual void f() { }; // virtual ~A(); // }; // @@ -92,7 +92,7 @@ public class NonVirtualDestructorCheckerTest extends CheckerTestCase { } // struct A { - // virtual void f() = 0; + // virtual void f() { }; // virtual ~A(); // ok. // }; // @@ -109,7 +109,7 @@ public class NonVirtualDestructorCheckerTest extends CheckerTestCase { } // struct A { - // virtual void f() = 0; + // virtual void f() { }; // ~A(); // warn! public non-virtual dtor. // // this affects B, D and E further down in the hierarchy as well // }; @@ -122,7 +122,25 @@ public class NonVirtualDestructorCheckerTest extends CheckerTestCase { // // struct E : public D { // }; - public void _testNonVirtualDtorInBaseClass2() { + public void testNonVirtualDtorInBaseClass2() { + loadCodeAndRun(getAboveComment()); checkErrorLines(3, 7, 11, 13); } + + // class A { // OK. Do _not_ warn here. + // // A is an abstract class because it has one pure virtual method. + // // A cannot be instantiated. + // virtual void f1() { }; + // virtual void f2() = 0; + // }; + // + // class B : public A { + // virtual void f1() { }; + // virtual void f2() { }; + // virtual ~B(); + // }; + public void testAbstractBaseClass() { + loadCodeAndRun(getAboveComment()); + checkNoErrors(); + } } |