From 600966533413e5e8cc77faab1fe8af33b50d059c Mon Sep 17 00:00:00 2001 From: Sergey Prigogin Date: Fri, 11 Mar 2016 17:47:11 -0500 Subject: [PATCH] Bug 489468 - Extract Function creates illegal declaration in .h when there is a using statement in the .cpp for an argument type Change-Id: Ie54ce13b434bab21f96b0c6bb7347846d52314e0 --- .../ExtractFunctionRefactoringTest.java | 81 ++++++++++++++++++- .../ui/refactoring/ClassMemberInserter.java | 52 +++++++++--- .../ExtractFunctionRefactoring.java | 77 +++++++++++++++++- 3 files changed, 191 insertions(+), 19 deletions(-) diff --git a/core/org.eclipse.cdt.ui.tests/ui/org/eclipse/cdt/ui/tests/refactoring/extractfunction/ExtractFunctionRefactoringTest.java b/core/org.eclipse.cdt.ui.tests/ui/org/eclipse/cdt/ui/tests/refactoring/extractfunction/ExtractFunctionRefactoringTest.java index 15cb4e3c377..6b30e36e1d1 100644 --- a/core/org.eclipse.cdt.ui.tests/ui/org/eclipse/cdt/ui/tests/refactoring/extractfunction/ExtractFunctionRefactoringTest.java +++ b/core/org.eclipse.cdt.ui.tests/ui/org/eclipse/cdt/ui/tests/refactoring/extractfunction/ExtractFunctionRefactoringTest.java @@ -16,8 +16,6 @@ import java.util.HashMap; import java.util.List; import java.util.Map; -import junit.framework.Test; - import org.eclipse.cdt.ui.PreferenceConstants; import org.eclipse.cdt.ui.tests.refactoring.RefactoringTestBase; @@ -27,6 +25,8 @@ import org.eclipse.cdt.internal.ui.refactoring.extractfunction.ExtractFunctionIn import org.eclipse.cdt.internal.ui.refactoring.extractfunction.ExtractFunctionRefactoring; import org.eclipse.cdt.internal.ui.refactoring.utils.VisibilityEnum; +import junit.framework.Test; + /** * Tests for Extract Function refactoring. */ @@ -35,12 +35,12 @@ public class ExtractFunctionRefactoringTest extends RefactoringTestBase { private String extractedFunctionName = "extracted"; private String returnValue; // Map from old names to new ones. - private Map parameterRename = new HashMap<>(); + private final Map parameterRename = new HashMap<>(); // New positions of parameters, or null. private int[] parameterOrder; private VisibilityEnum visibility = VisibilityEnum.v_private; private boolean virtual; - private boolean replaceDuplicates = true; + private final boolean replaceDuplicates = true; private ExtractFunctionRefactoring refactoring; public ExtractFunctionRefactoringTest() { @@ -570,6 +570,79 @@ public class ExtractFunctionRefactoringTest extends RefactoringTestBase { assertRefactoringSuccess(); } + //A.h + //#ifndef A_H_ + //#define A_H_ + // + //#include "B.h" + // + //class A { + //public: + // void foo(); + //}; + // + //#endif /*A_H_*/ + //==================== + //#ifndef A_H_ + //#define A_H_ + // + //#include "B.h" + // + //class A { + //public: + // void foo(); + // + //private: + // ns1::ns2::B extracted(ns1::ns2::B b); + //}; + // + //#endif /*A_H_*/ + + //A.cpp + //#include "A.h" + //#include "B.h" + // + //using ns1::ns2::B; + // + //void A::foo() { + // B b; + // /*$*/b.m();/*$$*/ + // b.n(); + //} + //==================== + //#include "A.h" + //#include "B.h" + // + //using ns1::ns2::B; + // + //B A::extracted(B b) { + // b.m(); + // return b; + //} + // + //void A::foo() { + // B b; + // b = extracted(b); + // b.n(); + //} + + //B.h + //#ifndef B_H_ + //#define B_H_ + // + //namespace ns1 { namespace ns2 { + //struct B { + // void m(); + // void n() const; + //}; + //}} + // + //#endif /*B_H_*/ + public void testUsingDeclaration() throws Exception { + getPreferenceStore().setValue(PreferenceConstants.FUNCTION_PASS_OUTPUT_PARAMETERS_BY_POINTER, true); + assertRefactoringSuccess(); + } + //A.cpp //void test() { // for (int i = 0; i < 2; i++) { diff --git a/core/org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/refactoring/ClassMemberInserter.java b/core/org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/refactoring/ClassMemberInserter.java index 713b048bed2..87027a7c74a 100644 --- a/core/org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/refactoring/ClassMemberInserter.java +++ b/core/org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/refactoring/ClassMemberInserter.java @@ -90,27 +90,57 @@ public class ClassMemberInserter { private ClassMemberInserter() { } - public static void createChange(ICPPASTCompositeTypeSpecifier classNode, + /** + * Inserts a node inside a class declaration. + * + * @param classNode the type specifier of the class declaration + * @param visibility desired visibility of the inserted declaration + * @param nodeToAdd the node to add + * @param isField whether the node being inserted is a field declaration or not + * @param collector the modification collector recording the insertion + * @return the rewriter for making further modifications to the inserted node + */ + public static ASTRewrite createChange(ICPPASTCompositeTypeSpecifier classNode, VisibilityEnum visibility, IASTNode nodeToAdd, boolean isField, ModificationCollector collector) { - createChange(classNode, visibility, Collections.singletonList(nodeToAdd), isField, collector); + List addedNodesRewrites = + createChange(classNode, visibility, Collections.singletonList(nodeToAdd), isField, collector); + return addedNodesRewrites.get(0); } - public static void createChange(ICPPASTCompositeTypeSpecifier classNode, + /** + * Inserts one more adjacent nodes inside a class declaration. + * + * @param classNode the type specifier of the class declaration + * @param visibility desired visibility of the inserted declarations + * @param nodesToAdd the nodes to add + * @param isField whether the nodes being inserted are field declaration or not + * @param collector the modification collector recording the insertion + * @return the rewriters for making further modifications to the inserted nodes + */ + public static List createChange(ICPPASTCompositeTypeSpecifier classNode, VisibilityEnum visibility, List nodesToAdd, boolean isField, ModificationCollector collector) { InsertionInfo info = findInsertionPoint(classNode, visibility, isField); - nodesToAdd = new ArrayList(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, + + if (info.getPrologue() != null) { + rewrite.insertBefore(info.getParentNode(), info.getInsertBeforeNode(), info.getPrologue(), createEditDescription(classNode)); } + + List addedNodeRewrites = new ArrayList<>(nodesToAdd.size()); + for (IASTNode node : nodesToAdd) { + addedNodeRewrites.add(rewrite.insertBefore(info.getParentNode(), info.getInsertBeforeNode(), node, + createEditDescription(classNode))); + } + + if (info.getEpilogue() != null) { + rewrite.insertBefore(info.getParentNode(), info.getInsertBeforeNode(), info.getEpilogue(), + createEditDescription(classNode)); + } + + return addedNodeRewrites; } public static InsertionInfo findInsertionPoint(ICPPASTCompositeTypeSpecifier classNode, diff --git a/core/org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/refactoring/extractfunction/ExtractFunctionRefactoring.java b/core/org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/refactoring/extractfunction/ExtractFunctionRefactoring.java index 8c48afa443b..f45f300cc19 100644 --- a/core/org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/refactoring/extractfunction/ExtractFunctionRefactoring.java +++ b/core/org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/refactoring/extractfunction/ExtractFunctionRefactoring.java @@ -13,6 +13,7 @@ package org.eclipse.cdt.internal.ui.refactoring.extractfunction; import java.util.ArrayList; +import java.util.Arrays; import java.util.HashMap; import java.util.HashSet; import java.util.List; @@ -34,6 +35,7 @@ import org.eclipse.osgi.util.NLS; import org.eclipse.text.edits.TextEditGroup; import org.eclipse.cdt.core.dom.ast.ASTVisitor; +import org.eclipse.cdt.core.dom.ast.DOMException; import org.eclipse.cdt.core.dom.ast.IASTBinaryExpression; import org.eclipse.cdt.core.dom.ast.IASTComment; import org.eclipse.cdt.core.dom.ast.IASTCompoundStatement; @@ -77,6 +79,7 @@ import org.eclipse.cdt.core.dom.ast.cpp.ICPPASTQualifiedName; import org.eclipse.cdt.core.dom.ast.cpp.ICPPASTTemplateDeclaration; import org.eclipse.cdt.core.dom.ast.cpp.ICPPASTTemplateId; import org.eclipse.cdt.core.dom.ast.cpp.ICPPASTTemplateParameter; +import org.eclipse.cdt.core.dom.ast.cpp.ICPPBinding; import org.eclipse.cdt.core.dom.ast.cpp.ICPPClassType; import org.eclipse.cdt.core.dom.ast.cpp.ICPPMethod; import org.eclipse.cdt.core.dom.rewrite.ASTRewrite; @@ -129,6 +132,7 @@ public class ExtractFunctionRefactoring extends CRefactoring { public static final String ID = "org.eclipse.cdt.internal.ui.refactoring.extractfunction.ExtractFunctionRefactoring"; //$NON-NLS-1$ + private static final String[] EMPTY_STRING_ARRAY = {}; static final Integer NULL_INTEGER = Integer.valueOf(0); private NodeContainer container; @@ -312,11 +316,11 @@ public class ExtractFunctionRefactoring extends CRefactoring { final IASTName methodName = new CPPASTName(info.getMethodName().toCharArray()); MethodContext context = info.getMethodContext(); - // Create declaration in class + // Create declaration in class. if (context.getType() == ContextType.METHOD && !context.isInline()) { createMethodDeclaration(methodName, context, collector); } - // Create method definition + // Create method definition. IASTNode firstNode = container.getNodesToWrite().get(0); createMethodDefinition(methodName, context, firstNode, collector); @@ -408,12 +412,77 @@ public class ExtractFunctionRefactoring extends CRefactoring { IASTSimpleDeclaration methodDeclaration = getDeclaration(collector, astMethodName); - ClassMemberInserter.createChange(classDeclaration, info.getVisibility(), + ASTRewrite rewrite = ClassMemberInserter.createChange(classDeclaration, info.getVisibility(), methodDeclaration, false, collector); + + // Names of external bindings may have to be qualified to be used in a header file. + if (classDeclaration.getTranslationUnit().isHeaderUnit()) + qualifyExternalReferences(methodDeclaration, classDeclaration, rewrite); + } + + private void qualifyExternalReferences(IASTNode node, ICPPASTCompositeTypeSpecifier classDeclaration, + final ASTRewrite rewrite) { + final ICPPBinding owner = (ICPPBinding) classDeclaration.getName().resolveBinding(); + final String[] contextQualifiers; + try { + contextQualifiers = owner.getQualifiedName(); + } catch (DOMException e) { + CUIPlugin.log(e); + return; + } + + node.accept(new ASTVisitor(true) { + @Override + public int visit(IASTName name) { + qualifyForContext((ICPPASTName) name, contextQualifiers, rewrite); + return PROCESS_SKIP; // Do non visit internals of qualified names. + } + }); + } + + private void qualifyForContext(ICPPASTName name, String[] contextQualifiers, ASTRewrite rewrite) { + ICPPASTName originalName = (ICPPASTName) name.getOriginalNode(); + IBinding binding = originalName.resolveBinding(); + try { + if (!(binding instanceof ICPPBinding)) + return; // Qualification is not needed. + String[] names = ((ICPPBinding) binding).getQualifiedName(); + names = removeCommonPrefix(names, contextQualifiers); + if (names.length <= 1) + return; // Qualification is not needed. + + ICPPASTQualifiedName qualifiedName; + if (name instanceof ICPPASTQualifiedName) { + qualifiedName = (ICPPASTQualifiedName) name; + if (qualifiedName.getQualifier().length >= names.length - 1) + return; // Qualified already. + } else { + qualifiedName = new CPPASTQualifiedName(name.copy(CopyStyle.withLocations)); + } + for (int i = 0; i < names.length - qualifiedName.getQualifier().length; i++) { + qualifiedName.addNameSpecifier(new CPPASTName(names[i].toCharArray())); + } + if (!(name instanceof ICPPASTQualifiedName)) + rewrite.replace(name, qualifiedName, null); + } catch (DOMException e) { + CUIPlugin.log(e); + return; + } + } + + private String[] removeCommonPrefix(String[] array1, String[] array2) { + for (int i = 0; i < array1.length && i < array2.length; i++) { + if (!array1[i].equals(array2[i])) { + if (i == 0) + return array1; + return Arrays.copyOfRange(array1, i, array1.length); + } + } + return EMPTY_STRING_ARRAY; } private void replaceSimilar(ModificationCollector collector, IASTName methodName) { - // Find similar code + // Find similar code. final List nodesToRewriteWithoutComments = getNodesWithoutComments(container.getNodesToWrite()); final List initTrail = getTrail(nodesToRewriteWithoutComments); IASTTranslationUnit ast = nodesToRewriteWithoutComments.get(0).getTranslationUnit();