diff --git a/codan/org.eclipse.cdt.codan.checkers/OSGI-INF/l10n/bundle.properties b/codan/org.eclipse.cdt.codan.checkers/OSGI-INF/l10n/bundle.properties index cd01cc4b343..a59ba9ebebb 100644 --- a/codan/org.eclipse.cdt.codan.checkers/OSGI-INF/l10n/bundle.properties +++ b/codan/org.eclipse.cdt.codan.checkers/OSGI-INF/l10n/bundle.properties @@ -19,10 +19,10 @@ checker.name.StatementHasNoEffect = StatementHasNoEffectChecker problem.description.StatementHasNoEffect = Finds statements like 'a;' or '-a;' or 'a-b;' which do no seems to have any side effect therefore suspicious problem.messagePattern.StatementHasNoEffect = Statement has no effect ''{0}'' problem.name.StatementHasNoEffect = Statement has no effect -checker.name.NonVirtualDescructor = NonVirtualDescructorChecker -problem.description.NonVirtualDescructor = If destructor is not declared virtual - destructor of derived class would not be called. -problem.messagePattern.NonVirtualDescructor = Class ''{0}'' has virtual method ''{1}'' but non-virtual destructor ''{2}'' -problem.name.NonVirtualDescructor = Class has a virtual method and non-virtual destructor +checker.name.NonVirtualDestructor = NonVirtualDestructorChecker +problem.description.NonVirtualDestructor = If destructor is not declared virtual - destructor of derived class would not be called. +problem.messagePattern.NonVirtualDestructor = Class ''{0}'' has virtual method ''{1}'' but non-virtual destructor +problem.name.NonVirtualDestructor = Class has a virtual method and non-virtual destructor checker.name.CatchByReference = CatchByReferenceChecker problem.description.CatchByReference = Catching by reference is recommended by C++ experts, "Throw by value, catch by reference". For one thing, this avoids copying and potentially slicing the exception. problem.messagePattern.CatchByReference = Catching by reference is recommended ''{0}'' diff --git a/codan/org.eclipse.cdt.codan.checkers/plugin.xml b/codan/org.eclipse.cdt.codan.checkers/plugin.xml index 91a6de728a9..b8e6b7f9150 100644 --- a/codan/org.eclipse.cdt.codan.checkers/plugin.xml +++ b/codan/org.eclipse.cdt.codan.checkers/plugin.xml @@ -34,15 +34,15 @@ + id="org.eclipse.cdt.codan.internal.checkers.NonVirtualDestructor" + name="%checker.name.NonVirtualDestructor"> + messagePattern="%problem.messagePattern.NonVirtualDestructor" + name="%problem.name.NonVirtualDestructor"> 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 5b5dbc39b4a..ead3e381331 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 @@ -44,9 +44,9 @@ public class NonVirtualDestructor extends AbstractIndexAstChecker { private IASTName className; private IBinding virtualMethod; private IBinding destructor; + private IASTDeclSpecifier warningLocation; OnEachClass() { - // shouldVisitDeclarations = true; shouldVisitDeclSpecifiers = true; } @@ -57,15 +57,19 @@ public class NonVirtualDestructor extends AbstractIndexAstChecker { if (err) { String clazz = className.toString(); String method = virtualMethod.getName(); - IASTNode ast = decl; + IASTNode node = decl; if (destructor != null) { if (destructor instanceof ICPPInternalBinding) { IASTNode[] decls = ((ICPPInternalBinding) destructor).getDeclarations(); - if (decls != null && decls.length > 0) - ast = decls[0]; + if (warningLocation == null) { + if (decls != null && decls.length > 0) + node = decls[0]; + } else { + node = warningLocation; + } } - reportProblem(ER_ID, ast, clazz, method, destructor.getName()); } + reportProblem(ER_ID, node, clazz, method); } } catch (DOMException e) { // Ignore, not an error. @@ -85,19 +89,20 @@ public class NonVirtualDestructor extends AbstractIndexAstChecker { ICPPASTCompositeTypeSpecifier spec = (ICPPASTCompositeTypeSpecifier) decl; className = spec.getName(); IBinding binding = className.resolveBinding(); - if (binding instanceof ICPPClassType) { + 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 hasOwnNonVirDestructor = false; + boolean hasOwnNonVirDestructor = false; // implicit destructor boolean hasDestructor = false; boolean hasVirtualMethod = false; for (ICPPMethod method : declaredMethods) { @@ -116,21 +121,31 @@ public class NonVirtualDestructor extends AbstractIndexAstChecker { boolean hasVirtualDestructor = false; // Class has own virtual method and own non-virtual destructor. if (hasOwnVirtualMethod && hasOwnNonVirDestructor) { - if (destructor instanceof ICPPMethod) { - // Check if dtor is public or is accessible by friends. - if (((ICPPMethod) destructor).getVisibility() != ICPPASTVisibilityLabel.v_public && - classType.getFriends().length == 0) { - return false; + 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 does have virtual methods and no destructors, compiler will generate implicit public destructor + if (hasOwnVirtualMethod && !hasDestructor) { + if (classType.getBases().length == 0) { + return true; } - // Check if one of its base classes has a virtual destructor. - return !hasVirtualDtorInBaseClass(classType); } // Class does not have virtual methods but has virtual destructor // - not an error if (!hasOwnVirtualMethod && hasDestructor && !hasOwnNonVirDestructor) { return false; } + // Check inherited methods ICPPMethod[] allDeclaredMethods = classType.getAllDeclaredMethods(); for (ICPPMethod method : allDeclaredMethods) { if (method.isVirtual() && !method.isDestructor()) { @@ -151,13 +166,23 @@ public class NonVirtualDestructor extends AbstractIndexAstChecker { if (hasOwnVirtualMethod) { // Class has own virtual method and base non-virtual destructor. if (hasDestructor && !hasVirtualDestructor) { - return true; + boolean noVirtualDtorInBase = !hasVirtualDtorInBaseClass(classType); + if (noVirtualDtorInBase) { + warningLocation = decl; + } + return noVirtualDtorInBase; } + } else if (hasVirtualMethod) { // Class has base virtual method and own non-virtual destructor. if (hasOwnNonVirDestructor) { return true; } + boolean noVirtualDtorInBase = !hasVirtualDtorInBaseClass(classType); + if (noVirtualDtorInBase) { + warningLocation = decl; + } + return noVirtualDtorInBase; } return false; } 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 5bdd871dea7..eaefcc8cc36 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 @@ -51,7 +51,7 @@ public class NonVirtualDestructorCheckerTest extends CheckerTestCase { // virtual void f() = 0; // ~A(); // warn! public non-virtual dtor. // }; - public void _testPublicVirtualDtorInClass() { + public void testPublicVirtualDtorInClass() { loadCodeAndRun(getAboveComment()); checkErrorLines(3); } @@ -60,7 +60,7 @@ public class NonVirtualDestructorCheckerTest extends CheckerTestCase { // virtual void f() = 0; // // warn! implicit public non-virtual dtor. // }; - public void _testImplicitPublicNonVirtualDtorInClass() { + public void testImplicitPublicNonVirtualDtorInClass() { loadCodeAndRun(getAboveComment()); checkErrorLines(1); } @@ -73,7 +73,7 @@ public class NonVirtualDestructorCheckerTest extends CheckerTestCase { // friend class F; // ~A(); // warn! can be called from class F. // }; - public void _testPublicNonVirtualDtorCanBeCalledFromFriendClass() { + public void testPublicNonVirtualDtorCanBeCalledFromFriendClass() { loadCodeAndRun(getAboveComment()); checkErrorLines(7); }