Bug 315528 - Non-virtual destructor diagnostics doesn't take superclass into account. Patch by Patrick Hofer.

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
+checker.name.NonVirtualDestructor = NonVirtualDestructorChecker
-problem.description.NonVirtualDescructor = If destructor is not declared virtual - destructor of derived class would not be called.
+problem.description.NonVirtualDestructor = 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.messagePattern.NonVirtualDestructor = Class ''{0}'' has virtual method ''{1}'' but non-virtual destructor
-problem.name.NonVirtualDescructor = Class has a virtual method and 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}''

codan/org.eclipse.cdt.codan.checkers/plugin.xml

@@ -34,15 +34,15 @@
       
       <checker
             class="org.eclipse.cdt.codan.internal.checkers.NonVirtualDestructor"
-            id="org.eclipse.cdt.codan.internal.checkers.NonVirtualDescructor"
+            id="org.eclipse.cdt.codan.internal.checkers.NonVirtualDestructor"
-            name="%checker.name.NonVirtualDescructor">
+            name="%checker.name.NonVirtualDestructor">
          <problem
                category="org.eclipse.cdt.codan.core.categories.ProgrammingProblems"
                defaultSeverity="Warning"
-               description="%problem.description.NonVirtualDescructor"
+               description="%problem.description.NonVirtualDestructor"
                id="org.eclipse.cdt.codan.internal.checkers.NonVirtualDestructorProblem"
-               messagePattern="%problem.messagePattern.NonVirtualDescructor"
+               messagePattern="%problem.messagePattern.NonVirtualDestructor"
-               name="%problem.name.NonVirtualDescructor">
+               name="%problem.name.NonVirtualDestructor">
          </problem>
       </checker>
 

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)
+								if (warningLocation == null) {
-									ast = decls[0];
+									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) {
+				if (classType.getFriends().length == 0) {
-					// Check if dtor is public or is accessible by friends.
+					if (destructor instanceof ICPPMethod) {
-					if (((ICPPMethod) destructor).getVisibility() != ICPPASTVisibilityLabel.v_public &&
+						// Check visibility of dtor. No error if it is protected or private.
-							classType.getFriends().length == 0) {
+						if (((ICPPMethod) destructor).getVisibility() != ICPPASTVisibilityLabel.v_public) {
-						return false;
+							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;
 		}

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);
 	}