Merge branch 'master' into sd90

codan/org.eclipse.cdt.codan.checkers/src/org/eclipse/cdt/codan/internal/checkers/NonVirtualDestructor.java

@@ -8,13 +8,12 @@
  * Contributors:
  *     Alena Laskavaia  - initial API and implementation
  *     Patrick Hofer [bug 315528]
+ *     Sergey Prigogin (Google)
  *******************************************************************************/
 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 +32,85 @@ 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;
+				}
+				ICPPClassType classType = (ICPPClassType) binding;
+				if (hasVirtualDestructor(classType)) {
+					return PROCESS_SKIP;
+				}
+				ICPPMethod virtualMethod = null;
+				for (ICPPMethod method : classType.getAllDeclaredMethods()) {
+					if (!method.isDestructor() && method.isVirtual()) {
+						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 != null) {
 				if (destructor instanceof ICPPInternalBinding) {
 					IASTNode[] decls = ((ICPPInternalBinding) destructor).getDeclarations();
-								if (warningLocation == null) {
+					if (decls != null && decls.length > 0) {
-									if (decls != null && decls.length > 0)
 						node = decls[0];
-								} else {
-									node = warningLocation;
 					}
 				}
-						}
+				reportProblem(PROBLEM_ID, node, className.getSimpleID().toString(),
-						reportProblem(ER_ID, node, clazz, method);
+						virtualMethod.getName());
-					}
-				} catch (DOMException e) {
-					// Ignore, not an error.
-				} catch (Exception e) {
-					CodanCheckersActivator.log(e);
-				}
 				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

@@ -7,6 +7,8 @@
  *
  * Contributors:
  *     Patrick Hofer - Initial API and implementation
+ *     Tomasz Wesolowski
+ *     Sergey Prigogin (Google)
  *******************************************************************************/
 package org.eclipse.cdt.codan.core.internal.checkers;
 
@@ -14,7 +16,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 +27,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 +88,7 @@ public class NonVirtualDestructorCheckerTest extends CheckerTestCase {
 	// struct B {
 	//   ~B(); // ok.
 	// };
-	public void testVirtualDtorInBaseClass() {
+	public void testVirtualDtorInBaseClass1() {
 		loadCodeAndRun(getAboveComment());
 		checkNoErrors();
 	}
@@ -108,6 +110,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,14 +140,12 @@ public class NonVirtualDestructorCheckerTest extends CheckerTestCase {
 	//
 	// struct E : public D {
 	// };
-	public void testNonVirtualDtorInBaseClass2() {
+	public void testNonVirtualDtorInBaseClass() {
 		loadCodeAndRun(getAboveComment());
 		checkErrorLines(3, 7, 11, 13);
 	}
 
-	// class A {  // OK. Do _not_ warn here.
+	// class A {
-	//   // A is an abstract class because it has one pure virtual method.
-	//   // A cannot be instantiated.
 	//   virtual void f1() { };
 	//   virtual void f2() = 0;
 	// };
@@ -141,6 +157,28 @@ public class NonVirtualDestructorCheckerTest extends CheckerTestCase {
 	// };
 	public void testAbstractBaseClass() {
 		loadCodeAndRun(getAboveComment());
-		checkNoErrors();
+		// It doesn't matter if the class is abstract or not - dtor can be called polymorphically.
+		checkErrorLines(1);
+	}
+
+	//	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);
 	}
 }

core/org.eclipse.cdt.core/model/org/eclipse/cdt/core/settings/model/ACSettingEntry.java

@@ -11,6 +11,7 @@
 package org.eclipse.cdt.core.settings.model;
 
 import org.eclipse.cdt.core.settings.model.util.LanguageSettingEntriesSerializer;
+import org.eclipse.cdt.internal.core.SafeStringInterner;
 
 
 
@@ -19,7 +20,7 @@ public abstract class ACSettingEntry implements ICSettingEntry {
 	String fName;
 	
 	ACSettingEntry(String name, int flags){
-		fName = name;
+		fName = SafeStringInterner.safeIntern(name);
 		fFlags = flags;
 	}
 

core/org.eclipse.cdt.core/model/org/eclipse/cdt/core/settings/model/CMacroEntry.java

@@ -10,6 +10,8 @@
  *******************************************************************************/
 package org.eclipse.cdt.core.settings.model;
 
+import org.eclipse.cdt.internal.core.SafeStringInterner;
+
 
 
 public final class CMacroEntry extends ACSettingEntry implements ICMacroEntry{
@@ -17,7 +19,7 @@ public final class CMacroEntry extends ACSettingEntry implements ICMacroEntry{
 
 	public CMacroEntry(String name, String value, int flags) {
 		super(name, flags);
-		fValue = value;
+		fValue = SafeStringInterner.safeIntern(value);
 		if(fValue == null)
 			fValue = "";	//$NON-NLS-1$
 	}

core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/dom/parser/cpp/CPPMethod.java

@@ -91,12 +91,12 @@ public class CPPMethod extends CPPFunction implements ICPPMethod {
 	 */
 	public int getVisibility() {
 		IASTDeclaration decl = getPrimaryDeclaration();
-		if( decl == null ){
+		if (decl == null) {
 			IScope scope = getScope();
-			if( scope instanceof ICPPClassScope ){
+			if (scope instanceof ICPPClassScope) {
 				ICPPClassType cls = ((ICPPClassScope)scope).getClassType();
-				if( cls != null )
+				if (cls != null)
-					return ( cls.getKey() == ICPPClassType.k_class ) ? ICPPASTVisibilityLabel.v_private : ICPPASTVisibilityLabel.v_public;
+					return (cls.getKey() == ICPPClassType.k_class) ? ICPPASTVisibilityLabel.v_private : ICPPASTVisibilityLabel.v_public;
 			}
 			return ICPPASTVisibilityLabel.v_private;
 		}
@@ -105,14 +105,15 @@ public class CPPMethod extends CPPFunction implements ICPPMethod {
 		IASTDeclaration [] members = cls.getMembers();
 		ICPPASTVisibilityLabel vis = null;
 		for (IASTDeclaration member : members) {
-			if( member instanceof ICPPASTVisibilityLabel )
+			if (member instanceof ICPPASTVisibilityLabel) {
 				vis = (ICPPASTVisibilityLabel) member;
-			else if( member == decl )
+			} else if (member == decl) {
 				break;
 			}
-		if( vis != null ){
+		}
+		if (vis != null) {
 			return vis.getVisibility();
-		} else if( cls.getKey() == ICPPASTCompositeTypeSpecifier.k_class ){
+		} else if (cls.getKey() == ICPPASTCompositeTypeSpecifier.k_class) {
 			return ICPPASTVisibilityLabel.v_private;
 		}
 		return ICPPASTVisibilityLabel.v_public;
@@ -145,9 +146,9 @@ public class CPPMethod extends CPPFunction implements ICPPMethod {
      */
     public boolean isVirtual() {
     	IASTDeclaration decl = getPrimaryDeclaration();
-		if( decl != null ){
+		if (decl != null) {
 			ICPPASTDeclSpecifier declSpec = getDeclSpec(decl);
-			if( declSpec != null ){
+			if (declSpec != null) {
 				return declSpec.isVirtual();
 			}
 		}
@@ -156,10 +157,11 @@ public class CPPMethod extends CPPFunction implements ICPPMethod {
 
 	protected ICPPASTDeclSpecifier getDeclSpec(IASTDeclaration decl) {
 		ICPPASTDeclSpecifier declSpec = null;
-		if( decl instanceof IASTSimpleDeclaration )
+		if (decl instanceof IASTSimpleDeclaration) {
 			declSpec = (ICPPASTDeclSpecifier) ((IASTSimpleDeclaration)decl).getDeclSpecifier();
-		else if( decl instanceof IASTFunctionDefinition )
+		} else if (decl instanceof IASTFunctionDefinition) {
 			declSpec = (ICPPASTDeclSpecifier) ((IASTFunctionDefinition)decl).getDeclSpecifier();
+		}
 		return declSpec;
 	}
 
@@ -169,9 +171,9 @@ public class CPPMethod extends CPPFunction implements ICPPMethod {
     @Override
 	public boolean isInline() {
         IASTDeclaration decl = getPrimaryDeclaration();
-        if( decl instanceof IASTFunctionDefinition )
+        if (decl instanceof IASTFunctionDefinition)
             return true;
-		if( decl == null )
+		if (decl == null)
 			return false;
 
         IASTDeclSpecifier declSpec = ((IASTSimpleDeclaration)decl).getDeclSpecifier();
@@ -183,7 +185,7 @@ public class CPPMethod extends CPPFunction implements ICPPMethod {
      */
     @Override
 	public boolean isMutable() {
-        return hasStorageClass( this, IASTDeclSpecifier.sc_mutable );
+        return hasStorageClass(this, IASTDeclSpecifier.sc_mutable);
     }
 
 	@Override

debug/org.eclipse.cdt.debug.core/src/org/eclipse/cdt/debug/internal/core/executables/StandardExecutableImporter.java

@@ -31,6 +31,7 @@ import org.eclipse.core.resources.IFile;
 import org.eclipse.core.resources.IFolder;
 import org.eclipse.core.resources.IProject;
 import org.eclipse.core.resources.IProjectDescription;
+import org.eclipse.core.resources.IResource;
 import org.eclipse.core.resources.IWorkspace;
 import org.eclipse.core.resources.ResourcesPlugin;
 import org.eclipse.core.runtime.CoreException;
@@ -143,7 +144,7 @@ public class StandardExecutableImporter implements IExecutableImporter {
 		for (int i = 0; i < segmentCount; i++) {
 			currentFolder = currentFolder.getFolder(new Path(path.segment(i)));
 			if (!currentFolder.exists()) {
-				((IFolder) currentFolder).create(false, true, new NullProgressMonitor());
+				((IFolder) currentFolder).create(IResource.VIRTUAL | IResource.DERIVED, true, new NullProgressMonitor());
 			}
 		}