Bug 315528 - [fp] Non-virtual destructor diagnostics doesn't take

superclass into account. Fix and additional tests including the ones
contributed by Tomasz Wesolowski.

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 static ICPPMethod getDestructor(ICPPClassType classType) {
-		private IASTName className;
+		for (ICPPMethod method : classType.getDeclaredMethods()) {
-		private IBinding virtualMethod;
+			if (method.isDestructor()) {
-		private IBinding destructor;
+				return method;
-		private IASTDeclSpecifier warningLocation;
+			}
+		}
+		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)) {
+			if (decl instanceof ICPPASTCompositeTypeSpecifier) {
-				try {
+				ICPPASTCompositeTypeSpecifier spec = (ICPPASTCompositeTypeSpecifier) decl;
-					boolean err = hasErrorCondition(decl);
+				IASTName className = spec.getName();
-					if (err) {
+				IBinding binding = className.resolveBinding();
-						String clazz = className.toString();
+				if (!(binding instanceof ICPPClassType)) {
-						String method = virtualMethod.getName();
+					return PROCESS_SKIP;
-						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);
 				}
+				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;
-		}
 	}
 }

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