Bug 423126 - Extract Function refactoring misses additional occurrences

of the extracted code

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

@@ -37,7 +37,7 @@ import org.eclipse.cdt.internal.core.dom.parser.cpp.semantics.CPPVisitor;
  * Scope of a function, containing labels.
  */
 public class CPPFunctionScope extends CPPScope implements ICPPFunctionScope {
-    private CharArrayObjectMap<IBinding> labels = CharArrayObjectMap.emptyMap();
+    private CharArrayObjectMap<ILabel> labels = CharArrayObjectMap.emptyMap();
     
 	/**
 	 * @param physicalNode
@@ -56,22 +56,18 @@ public class CPPFunctionScope extends CPPScope implements ICPPFunctionScope {
 	    //3.3.4 only labels have function scope
 	    if (!(binding instanceof ILabel))
 	        return;
-	    
-	    if (labels == CharArrayObjectMap.EMPTY_MAP)
-	        labels = new CharArrayObjectMap<IBinding>(2);
-	    
-	    labels.put(binding.getNameCharArray(), binding);
-	}
 
-	public IBinding getBinding(IASTName name) {
+	    if (labels == CharArrayObjectMap.EMPTY_MAP)
-	    return labels.get(name.getLookupKey());
+	        labels = new CharArrayObjectMap<ILabel>(2);
+
+	    labels.put(binding.getNameCharArray(), (ILabel) binding);
 	}
 
 	@Override
 	public IBinding[] find(String name) {
 	    char[] n = name.toCharArray();
 	    List<IBinding> bindings = new ArrayList<IBinding>();
-	    
+
 	    for (int i = 0; i < labels.size(); i++) {
 	    	char[] key = labels.keyAt(i);
 	    	if (CharArrayUtils.equals(key, n)) {

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

@@ -23,11 +23,11 @@ import org.eclipse.cdt.internal.core.dom.parser.cpp.semantics.CPPVisitor;
 import org.eclipse.core.runtime.PlatformObject;
 
 public class CPPLabel extends PlatformObject implements ILabel, ICPPInternalBinding {
-    private IASTName statement;
+    private IASTName name;
 
-    public CPPLabel(IASTName statement) {
+    public CPPLabel(IASTName name) {
-        this.statement = statement;
+        this.name = name;
-        statement.setBinding(this);
+        name.setBinding(this);
     }
 
     @Override
@@ -37,11 +37,12 @@ public class CPPLabel extends PlatformObject implements ILabel, ICPPInternalBind
 
     @Override
 	public IASTNode getDefinition() {
-        return statement;
+        return name;
     }
 
     @Override
 	public IASTLabelStatement getLabelStatement() {
+    	IASTNode statement = name.getParent();
         if (statement instanceof IASTLabelStatement)
             return (IASTLabelStatement) statement;
         
@@ -56,20 +57,20 @@ public class CPPLabel extends PlatformObject implements ILabel, ICPPInternalBind
 
     @Override
 	public char[] getNameCharArray() {
-        return statement.getSimpleID();
+        return name.getSimpleID();
     }
 
     @Override
 	public IScope getScope() {
-        return CPPVisitor.getContainingScope(statement);
+        return CPPVisitor.getContainingScope(name);
     }
 
     public IASTNode getPhysicalNode() {
-        return statement;
+        return name;
     }
 
     public void setLabelStatement(IASTName labelStatement) {
-        statement = labelStatement;
+        name = labelStatement;
     }
 
     @Override
@@ -102,7 +103,7 @@ public class CPPLabel extends PlatformObject implements ILabel, ICPPInternalBind
 
 	@Override
 	public IBinding getOwner() {
-		return CPPVisitor.findEnclosingFunction(statement);
+		return CPPVisitor.findEnclosingFunction(name);
 	}
 
 	@Override

core/org.eclipse.cdt.ui.tests/ui/org/eclipse/cdt/ui/tests/refactoring/extractfunction/ExtractFunctionRefactoringTest.java

@@ -2919,6 +2919,7 @@ public class ExtractFunctionRefactoringTest extends RefactoringTestBase {
 	public void testDuplicates() throws Exception {
 		assertRefactoringSuccess();
 	}
+
 	//A.h
 	//#ifndef A_H_
 	//#define A_H_
@@ -4192,6 +4193,78 @@ public class ExtractFunctionRefactoringTest extends RefactoringTestBase {
 		assertRefactoringSuccess();
 	}
 
+	//test.cpp
+	//template<typename T>
+	//void p(T p) {}
+	//
+	//#define TRACE(var) p(__LINE__), p(": "), p(#var), p("="), p(var)
+	//
+	//void test(int x, int y) {
+	//	/*$*/TRACE(x);/*$$*/
+	//	TRACE(y);
+	//	TRACE(x);
+	//}
+	//====================
+	//template<typename T>
+	//void p(T p) {}
+	//
+	//#define TRACE(var) p(__LINE__), p(": "), p(#var), p("="), p(var)
+	//
+	//void extracted(int x) {
+	//	TRACE(x);
+	//}
+	//
+	//void test(int x, int y) {
+	//	extracted(x);
+	//	TRACE(y);
+	//	extracted(x);
+	//}
+	public void testLiteralFromMacro() throws Exception {
+		assertRefactoringSuccess();
+	}
+
+	//test.cpp
+	//#define LABEL(a, b) a ## b
+	//#define MACRO1(cond1, cond2, var, n) \
+	//if (cond1) { \
+	//  if (cond2) { \
+	//    var++; \
+	//    goto LABEL(label, n); \
+	//  } \
+	//} else LABEL(label, n): \
+	//  var--
+	//#define MACRO(var) MACRO1(true, false, var, __COUNTER__)
+	//
+	//void test1(int x, int y) {
+	//  MACRO(x);
+	//  MACRO(y);
+	//  /*$*/MACRO(x);/*$$*/
+	//}
+	//====================
+	//#define LABEL(a, b) a ## b
+	//#define MACRO1(cond1, cond2, var, n) \
+	//if (cond1) { \
+	//  if (cond2) { \
+	//    var++; \
+	//    goto LABEL(label, n); \
+	//  } \
+	//} else LABEL(label, n): \
+	//  var--
+	//#define MACRO(var) MACRO1(true, false, var, __COUNTER__)
+	//
+	//void extracted(int x) {
+	//	MACRO(x);
+	//}
+	//
+	//void test1(int x, int y) {
+	//	extracted(x);
+	//  MACRO(y);
+	//	extracted(x);
+	//}
+	public void testLabelFromMacro() throws Exception {
+		assertRefactoringSuccess();
+	}
+
 	//A.h
 	//#ifndef A_H_
 	//#define A_H_

core/org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/refactoring/extractfunction/ExtractFunctionRefactoring.java

@@ -703,15 +703,19 @@ public class ExtractFunctionRefactoring extends CRefactoring {
 		boolean theRetName = false;
 
 		for (NameInformation nameInfo : info.getParameters()) {
-			Integer trailSeqNumber = trailNameTable.get(nameInfo.getDeclarationName().getRawSignature());
 			String origName = null;
-			for (Entry<String, Integer> entry : similarNameTable.entrySet()) {
+			Integer trailSeqNumber = trailNameTable.get(nameInfo.getDeclarationName().getRawSignature());
-				if (entry.getValue().equals(trailSeqNumber)) {
+			if (trailSeqNumber != null) {
-					origName = entry.getKey();
+				for (Entry<String, Integer> entry : similarNameTable.entrySet()) {
-					if (info.getReturnVariable() != null &&	trailSeqNumber.intValue() == returnNumber) {
+					if (entry.getValue().equals(trailSeqNumber)) {
-						theRetName = true;
+						origName = entry.getKey();
+						if (info.getReturnVariable() != null &&	trailSeqNumber.intValue() == returnNumber) {
+							theRetName = true;
+						}
 					}
 				}
+			} else {
+				origName = String.valueOf(nameInfo.getDeclarationName().getSimpleID());
 			}
 
 			if (origName != null) {
@@ -773,7 +777,9 @@ public class ExtractFunctionRefactoring extends CRefactoring {
 	private IASTNode getReturnAssignment(IASTExpressionStatement stmt,
 			IASTFunctionCallExpression callExpression, IASTName retname) {
 		if (info.getReturnVariable().equals(info.getMandatoryReturnVariable())) {
-			IASTSimpleDeclaration orgDecl = CPPVisitor.findAncestorWithType(info.getReturnVariable().getDeclarationName(), IASTSimpleDeclaration.class);
+			IASTSimpleDeclaration orgDecl =
+					CPPVisitor.findAncestorWithType(info.getReturnVariable().getDeclarationName(),
+							IASTSimpleDeclaration.class);
 			IASTSimpleDeclaration decl = new CPPASTSimpleDeclaration();
 
 			decl.setDeclSpecifier(orgDecl.getDeclSpecifier().copy(CopyStyle.withLocations));
@@ -804,8 +810,7 @@ public class ExtractFunctionRefactoring extends CRefactoring {
 		return getReturnAssignment(stmt, binaryExpression);
 	}
 
-	private IASTNode getReturnAssignment(IASTExpressionStatement stmt,
+	private IASTNode getReturnAssignment(IASTExpressionStatement stmt, IASTExpression callExpression) {
-			IASTExpression callExpression) {
 		IASTNode node = container.getNodesToWrite().get(0);
 		return extractor.createReturnAssignment(node, stmt, callExpression);
 	}

core/org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/refactoring/extractfunction/TrailNodeEqualityChecker.java

@@ -41,6 +41,7 @@ import org.eclipse.cdt.core.dom.ast.IASTStatement;
 import org.eclipse.cdt.core.dom.ast.IASTTypeIdExpression;
 import org.eclipse.cdt.core.dom.ast.IASTUnaryExpression;
 import org.eclipse.cdt.core.dom.ast.IBinding;
+import org.eclipse.cdt.core.dom.ast.ILabel;
 import org.eclipse.cdt.core.dom.ast.IType;
 import org.eclipse.cdt.core.dom.ast.c.ICASTDeclSpecifier;
 import org.eclipse.cdt.core.dom.ast.cpp.ICPPASTCatchHandler;
@@ -121,7 +122,7 @@ public class TrailNodeEqualityChecker implements EqualityChecker<IASTNode> {
 			return trailName.equals(name);
 		} else if (trailNode instanceof TrailName && node instanceof IASTName) {
 			TrailName trailName = (TrailName) trailNode;
-			IASTName name = (IASTName)node;
+			IASTName name = (IASTName) node;
 			return isNameEqual(trailName, name);
 		} else {
 			return true;
@@ -275,7 +276,7 @@ public class TrailNodeEqualityChecker implements EqualityChecker<IASTNode> {
 		} else if (trailNode instanceof IASTLiteralExpression) {
 			IASTLiteralExpression trailLiteral = (IASTLiteralExpression) trailNode;
 			IASTLiteralExpression literal = (IASTLiteralExpression) node;
-			return trailLiteral.getKind() == literal.getKind() && trailLiteral.toString().equals(literal.toString());
+			return trailLiteral.getKind() == literal.getKind() && trailLiteral.getRawSignature().equals(literal.getRawSignature());
 		} else if (trailNode instanceof IASTUnaryExpression) {
 			IASTUnaryExpression trailExpr = (IASTUnaryExpression) trailNode;
 			IASTUnaryExpression expr = (IASTUnaryExpression) node;
@@ -359,9 +360,10 @@ public class TrailNodeEqualityChecker implements EqualityChecker<IASTNode> {
 			return false;
 		}
 
+		IASTName realName = trailName.getRealName();
+		IBinding realBind = realName.resolveBinding();
+		IBinding nameBind = name.resolveBinding();
 		if (trailName.isGloballyQualified()) {
-			IBinding realBind = trailName.getRealName().resolveBinding();
-			IBinding nameBind = name.resolveBinding();
 			IIndexName[] realDecs;
 			IIndexName[] nameDecs;
 			try {
@@ -371,25 +373,30 @@ public class TrailNodeEqualityChecker implements EqualityChecker<IASTNode> {
 				CUIPlugin.log(e);
 				return false;
 			}
-			if (realDecs.length == nameDecs.length) {
-				for (int i = 0; i < realDecs.length; ++i) {
-					IASTFileLocation rfl = realDecs[i].getFileLocation();
-					IASTFileLocation nfl = nameDecs[i].getFileLocation();
-					if (rfl.getNodeOffset() != nfl.getNodeOffset() || !rfl.getFileName().equals(nfl.getFileName()))
-						return false;
-				}
-				return true;
-			} else {
-				return false;
-			}
-		} else {
-			IType oType = getType(trailName.getRealName().resolveBinding());
-			IType nType = getType(name.resolveBinding());
-			if (oType == null || nType == null)
-				return false;
 
-			if (oType.isSameType(nType))
+			if (realDecs.length != nameDecs.length)
-				return true;
+				return false;
+			for (int i = 0; i < realDecs.length; ++i) {
+				IASTFileLocation rfl = realDecs[i].getFileLocation();
+				IASTFileLocation nfl = nameDecs[i].getFileLocation();
+				if (rfl.getNodeOffset() != nfl.getNodeOffset() || !rfl.getFileName().equals(nfl.getFileName()))
+					return false;
+			}
+			return true;
+		} else {
+			if (realBind instanceof ILabel && nameBind instanceof ILabel) {
+				if (realName.getRawSignature().equals(name.getRawSignature())) {
+					return true;
+				}
+			} else {
+				IType oType = getType(realBind);
+				IType nType = getType(nameBind);
+				if (oType == null || nType == null)
+					return false;
+	
+				if (oType.isSameType(nType))
+					return true;
+			}
 		}
 		return false;
 	}