Proper handling of visibility for extracted inline functions.

core/org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/refactoring/ClassMemberInserter.java

@@ -48,6 +48,47 @@ import org.eclipse.cdt.internal.ui.refactoring.utils.VisibilityEnum;
  * @author Mirko Stocker
  */
 public class ClassMemberInserter {
+	public static class InsertionInfo {
+		private final IASTNode parentNode;
+		/**
+		 * The node before which the new node should be inserted. A null value indicates insertion
+		 * to the end of parentNode
+		 */
+		private IASTNode insertBeforeNode; //
+		/**  Visibility label to insert before the new node or null. */
+		private ICPPASTVisibilityLabel prologue;
+		/**  Visibility label to insert after the new node or null. */
+		private ICPPASTVisibilityLabel epilogue;
+
+		public InsertionInfo(IASTNode parentNode, IASTNode insertBeforeNode) {
+			this.parentNode = parentNode;
+			this.insertBeforeNode = insertBeforeNode;
+		}
+
+		public InsertionInfo(IASTNode parentNode) {
+			this(parentNode, null);
+		}
+
+		public IASTNode getParentNode() {
+			return parentNode;
+		}
+
+		public IASTNode getInsertBeforeNode() {
+			return insertBeforeNode;
+		}
+
+		public ICPPASTVisibilityLabel getPrologue() {
+			return prologue;
+		}
+
+		public ICPPASTVisibilityLabel getEpilogue() {
+			return epilogue;
+		}
+	}
+
+	// Not instantiatable. All methods are static.
+	private ClassMemberInserter() {
+	}
 
 	public static void createChange(ICPPASTCompositeTypeSpecifier classNode,
 			VisibilityEnum visibility, IASTNode nodeToAdd, boolean isField,
@@ -58,7 +99,23 @@ public class ClassMemberInserter {
 	public static void createChange(ICPPASTCompositeTypeSpecifier classNode,
 			VisibilityEnum visibility, List<IASTNode> nodesToAdd, boolean isField,
 			ModificationCollector collector) {
+		InsertionInfo info = findInsertionPoint(classNode, visibility, isField);
 		nodesToAdd = new ArrayList<IASTNode>(nodesToAdd);
+		if (info.getPrologue() != null)
+			nodesToAdd.add(0, info.getPrologue());
+		if (info.getEpilogue() != null)
+			nodesToAdd.add(info.getEpilogue());
+
+		ASTRewrite rewrite = collector.rewriterForTranslationUnit(classNode.getTranslationUnit());
+		for (IASTNode node : nodesToAdd) {
+			rewrite.insertBefore(info.getParentNode(), info.getInsertBeforeNode(), node,
+					createEditDescription(classNode));
+		}
+	}
+
+	public static InsertionInfo findInsertionPoint(ICPPASTCompositeTypeSpecifier classNode,
+			VisibilityEnum visibility, boolean isField) {
+		InsertionInfo info = new InsertionInfo(classNode);
 		VisibilityEnum defaultVisibility = classNode.getKey() == IASTCompositeTypeSpecifier.k_struct ?
 				VisibilityEnum.v_public : VisibilityEnum.v_private;
 		VisibilityEnum currentVisibility = defaultVisibility;
@@ -105,24 +162,19 @@ public class ClassMemberInserter {
 		if (index < 0)
 			index = lastPrecedingVisibilityIndex;
 		index++;
-		IASTNode nextNode = index < members.length ? members[index] : null;
+		if (index < members.length)
+			info.insertBeforeNode = members[index];
 
 		if (lastMatchingVisibilityIndex < 0 &&
-				!(index == 0 && classNode.getKey() == IASTCompositeTypeSpecifier.k_struct && visibility == defaultVisibility)) {
+				!(index == 0 && classNode.getKey() == IASTCompositeTypeSpecifier.k_struct &&
-			nodesToAdd.add(0, new CPPASTVisibilityLabel(visibility.getVisibilityLabelValue()));
+						visibility == defaultVisibility)) {
-			if (index == 0 && nextNode != null && !(nextNode instanceof ICPPASTVisibilityLabel)) {
+			info.prologue = new CPPASTVisibilityLabel(visibility.getVisibilityLabelValue());
-				nodesToAdd.add(new CPPASTVisibilityLabel(defaultVisibility.getVisibilityLabelValue()));
+			if (index == 0 && info.insertBeforeNode != null &&
+					!(info.insertBeforeNode instanceof ICPPASTVisibilityLabel)) {
+				info.epilogue = new CPPASTVisibilityLabel(defaultVisibility.getVisibilityLabelValue());
 			}
 		}
-
+		return info;
-		ASTRewrite rewrite = collector.rewriterForTranslationUnit(classNode.getTranslationUnit());
-		for (IASTNode node : nodesToAdd) {
-			rewrite.insertBefore(classNode, nextNode, node, createEditDescription(classNode));
-		}
-	}	
-
-	// Not instantiatable. All methods are static.
-	private ClassMemberInserter() {
 	}
 
 	private static TextEditGroup createEditDescription(ICPPASTCompositeTypeSpecifier classNode) {

core/org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/refactoring/MethodContext.java

@@ -26,8 +26,8 @@ import org.eclipse.cdt.internal.core.dom.parser.ASTInternal;
 import org.eclipse.cdt.internal.core.dom.parser.cpp.ICPPInternalBinding;
 
 /**
- * Represents a function or method and adds some useful helper methods to
+ * Represents a function or method and adds some useful helper methods to determine
- * determine if methods are in the same class.
+ * if two methods are in the same class.
  */
 public class MethodContext {
 	public enum ContextType { NONE, FUNCTION, METHOD }

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

@@ -38,8 +38,8 @@ public class ExtractFunctionInformation {
 	private boolean virtual;
 
 	/**
-	 * Returns the function declarator of the method / function from were the statements
+	 * Returns the function declarator of the method or function from were the statements
-	 * are extacted from.
+	 * are extracted from.
 	 * @return the function declarator or null
 	 */
 	public ICPPASTFunctionDeclarator getDeclarator() {

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

@@ -108,6 +108,7 @@ import org.eclipse.cdt.internal.core.dom.rewrite.astwriter.ASTWriterVisitor;
 import org.eclipse.cdt.internal.ui.refactoring.CRefactoring;
 import org.eclipse.cdt.internal.ui.refactoring.CRefactoringDescriptor;
 import org.eclipse.cdt.internal.ui.refactoring.ClassMemberInserter;
+import org.eclipse.cdt.internal.ui.refactoring.ClassMemberInserter.InsertionInfo;
 import org.eclipse.cdt.internal.ui.refactoring.Container;
 import org.eclipse.cdt.internal.ui.refactoring.MethodContext;
 import org.eclipse.cdt.internal.ui.refactoring.MethodContext.ContextType;
@@ -379,9 +380,10 @@ public class ExtractFunctionRefactoring extends CRefactoring {
 	}
 
 	private void createMethodDefinition(final IASTName methodName, MethodContext context,
-			IASTNode firstNode,	ModificationCollector collector) {
+			IASTNode firstExtractedNode, ModificationCollector collector) {
-		IASTFunctionDefinition node = CPPVisitor.findAncestorWithType(firstNode, IASTFunctionDefinition.class); 
+		IASTFunctionDefinition functionToExtractFrom =
-		if (node != null) { 
+				CPPVisitor.findAncestorWithType(firstExtractedNode, IASTFunctionDefinition.class);
+		if (functionToExtractFrom != null) {
 			String title;
 			if (context.getType() == MethodContext.ContextType.METHOD) {
 				title = Messages.ExtractFunctionRefactoring_CreateMethodDef;
@@ -389,8 +391,8 @@ public class ExtractFunctionRefactoring extends CRefactoring {
 				title = Messages.ExtractFunctionRefactoring_CreateFunctionDef;
 			}
 
-			ASTRewrite rewriter = collector.rewriterForTranslationUnit(node.getTranslationUnit());
+			ASTRewrite rewriter = collector.rewriterForTranslationUnit(functionToExtractFrom.getTranslationUnit());
-			addMethod(methodName, context, rewriter, node, new TextEditGroup(title));
+			addMethod(methodName, context, rewriter, functionToExtractFrom, new TextEditGroup(title));
 		}
 	}
 
@@ -584,7 +586,7 @@ public class ExtractFunctionRefactoring extends CRefactoring {
 	}
 
 	private void addMethod(IASTName methodName, MethodContext context, ASTRewrite rewrite,
-			IASTNode insertPoint, TextEditGroup group) {
+			IASTNode functionToExtractFrom, TextEditGroup group) {
 		ICPPASTQualifiedName qname = new CPPASTQualifiedName();
 		if (context.getType() == ContextType.METHOD) {
 			if (context.getMethodQName() != null) {
@@ -611,7 +613,8 @@ public class ExtractFunctionRefactoring extends CRefactoring {
 		func.setBody(compound);
 
 		ASTRewrite subRewrite;
-		IASTNode parent = insertPoint.getParent();
+		IASTNode parent = functionToExtractFrom.getParent();
+		IASTNode nodeToInsert = func;
 		if (parent instanceof ICPPASTTemplateDeclaration) {
 			ICPPASTTemplateDeclaration parentTemplate = (ICPPASTTemplateDeclaration) parent;
 			CPPASTTemplateDeclaration templateDeclaration = new CPPASTTemplateDeclaration();
@@ -621,10 +624,33 @@ public class ExtractFunctionRefactoring extends CRefactoring {
 				templateDeclaration.addTemplateParameter(param.copy(CopyStyle.withLocations));
 			}
 
+			functionToExtractFrom = parentTemplate;
 			templateDeclaration.setDeclaration(func);
-			subRewrite = rewrite.insertBefore(parent.getParent(), parent, templateDeclaration, group);
+			nodeToInsert = templateDeclaration;
+			parent = parent.getParent();
+		}
+
+		InsertionInfo insertion;
+		if (parent instanceof ICPPASTCompositeTypeSpecifier) {
+			// Inserting into a class declaration
+			insertion = ClassMemberInserter.findInsertionPoint((ICPPASTCompositeTypeSpecifier) parent,
+					info.getVisibility(), false);
 		} else {
-			subRewrite = rewrite.insertBefore(parent, insertPoint, func, group);
+			// Inserting into a translation unit or a namespace.
+			// TODO(sprigogin): Use insertBeforeNode instead of functionToExtractFrom when creating InsertionInfo
+//			IASTNode insertBeforeNode = info.getMethodContext().getType() == ContextType.METHOD ?
+//					null : functionToExtractFrom;
+			insertion = new InsertionInfo(parent, functionToExtractFrom);
+		}
+		if (insertion.getPrologue() != null) {
+			rewrite.insertBefore(insertion.getParentNode(),
+					insertion.getInsertBeforeNode(), insertion.getPrologue(), group);
+		}
+		subRewrite = rewrite.insertBefore(insertion.getParentNode(),
+				insertion.getInsertBeforeNode(), nodeToInsert, group);
+		if (insertion.getEpilogue() != null) {
+			rewrite.insertBefore(insertion.getParentNode(),
+					insertion.getInsertBeforeNode(), insertion.getEpilogue(), group);
 		}
 
 		extractor.constructMethodBody(compound, container.getNodesToWrite(),