From 21e056c85659a76f88f4a26de0e0704c9ed1756a Mon Sep 17 00:00:00 2001 From: Sergey Prigogin Date: Sun, 17 Jul 2011 17:20:16 -0700 Subject: Bug 315528 - [fp] Non-virtual destructor diagnostics doesn't take superclass into account. Fix and additional tests including the ones contributed by Tomasz Wesolowski. --- .../internal/checkers/NonVirtualDestructor.java | 216 ++++++--------------- 1 file changed, 60 insertions(+), 156 deletions(-) (limited to 'codan/org.eclipse.cdt.codan.checkers') 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; - - OnEachClass() { - shouldVisitDeclSpecifiers = true; + private static ICPPMethod getDestructor(ICPPClassType classType) { + for (ICPPMethod method : classType.getDeclaredMethods()) { + if (method.isDestructor()) { + return method; + } } + return null; + } - 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); + 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 PROCESS_SKIP; } - return PROCESS_CONTINUE; } + return false; + } - /** - * @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; + private class OnEachClass extends ASTVisitor { + OnEachClass() { + shouldVisitDeclSpecifiers = true; + } + + public int visit(IASTDeclSpecifier decl) { + if (decl instanceof ICPPASTCompositeTypeSpecifier) { + ICPPASTCompositeTypeSpecifier spec = (ICPPASTCompositeTypeSpecifier) decl; + IASTName className = spec.getName(); + IBinding binding = className.resolveBinding(); + if (!(binding instanceof ICPPClassType)) { + return PROCESS_SKIP; } - if (method.isDestructor()) { - hasDestructor = true; - if (!method.isVirtual()) { - hasOwnNonVirtualDestructor = true; - destructor = method; - } + ICPPClassType classType = (ICPPClassType) binding; + if (hasVirtualDestructor(classType)) { + return PROCESS_SKIP; } - } - 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) + 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 (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; + if (virtualMethod == null) { + return PROCESS_SKIP; } - boolean noVirtualDtorInBase = !hasVirtualDtorInBaseClass(classType); - if (noVirtualDtorInBase) { - warningLocation = decl; + 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; } - 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; + IASTNode node = decl; + if (destructor instanceof ICPPInternalBinding) { + IASTNode[] decls = ((ICPPInternalBinding) destructor).getDeclarations(); + if (decls != null && decls.length > 0) { + node = decls[0]; } } - if (hasVirtualDtorInBaseClass(testedBaseClass)) - return true; + reportProblem(PROBLEM_ID, node, className.getSimpleID().toString(), + virtualMethod.getName()); + return PROCESS_SKIP; } - return false; - } - - private boolean isClassDecl(IASTDeclSpecifier decl) { - return decl instanceof ICPPASTCompositeTypeSpecifier; + return PROCESS_CONTINUE; } } } -- cgit v1.2.3