From 73337a5882a8c46bd708688b4963794a9a9c0bd9 Mon Sep 17 00:00:00 2001 From: sprigogin Date: Mon, 26 Mar 2012 19:44:13 -0400 Subject: [PATCH] Proper handling of visibility for extracted inline functions. --- .../ui/refactoring/ClassMemberInserter.java | 108 +++++++++++++----- .../ui/refactoring/MethodContext.java | 56 ++++----- .../ExtractFunctionInformation.java | 20 ++-- .../ExtractFunctionRefactoring.java | 90 +++++++++------ 4 files changed, 176 insertions(+), 98 deletions(-) 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 d64bf24bf0d..92e235c3e91 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 @@ -1,12 +1,12 @@ /******************************************************************************* - * Copyright (c) 2007, 2011 Institute for Software, HSR Hochschule fuer Technik + * Copyright (c) 2007, 2011 Institute for Software, HSR Hochschule fuer Technik * Rapperswil, University of applied sciences and others - * All rights reserved. This program and the accompanying materials - * are made available under the terms of the Eclipse Public License v1.0 - * which accompanies this distribution, and is available at - * http://www.eclipse.org/legal/epl-v10.html + * All rights reserved. This program and the accompanying materials + * are made available under the terms of the Eclipse Public License v1.0 + * which accompanies this distribution, and is available at + * http://www.eclipse.org/legal/epl-v10.html * - * Contributors: + * Contributors: * Institute for Software - initial API and implementation * Sergey Prigogin (Google) *******************************************************************************/ @@ -42,13 +42,54 @@ import org.eclipse.cdt.internal.core.dom.parser.cpp.CPPASTVisibilityLabel; import org.eclipse.cdt.internal.ui.refactoring.utils.VisibilityEnum; /** - * Adds a declaration to an existing class via the ModificationCollector. Automatically determines + * Adds a declaration to an existing class via the ModificationCollector. Automatically determines * an appropriate insertion point for the desired visibility. - * + * * @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, ModificationCollector collector) { @@ -58,7 +99,23 @@ public class ClassMemberInserter { public static void 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, + 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; @@ -69,13 +126,13 @@ public class ClassMemberInserter { int lastMatchingVisibilityIndex = -1; int lastPrecedingVisibilityIndex = -1; IASTDeclaration[] members = classNode.getMembers(); - - // Find the insert location by iterating over the elements of the class + + // Find the insert location by iterating over the elements of the class // and remembering the last element with the matching visibility and the last element - // with preceding visibility (according to the visibility order preference). + // with preceding visibility (according to the visibility order preference). for (int i = 0; i < members.length; i++) { IASTDeclaration declaration = members[i]; - + if (declaration instanceof ICPPASTVisibilityLabel) { currentVisibility = VisibilityEnum.from((ICPPASTVisibilityLabel) declaration); } @@ -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)) { - nodesToAdd.add(0, new CPPASTVisibilityLabel(visibility.getVisibilityLabelValue())); - if (index == 0 && nextNode != null && !(nextNode instanceof ICPPASTVisibilityLabel)) { - nodesToAdd.add(new CPPASTVisibilityLabel(defaultVisibility.getVisibilityLabelValue())); + !(index == 0 && classNode.getKey() == IASTCompositeTypeSpecifier.k_struct && + visibility == defaultVisibility)) { + info.prologue = new CPPASTVisibilityLabel(visibility.getVisibilityLabelValue()); + if (index == 0 && info.insertBeforeNode != null && + !(info.insertBeforeNode instanceof ICPPASTVisibilityLabel)) { + info.epilogue = new CPPASTVisibilityLabel(defaultVisibility.getVisibilityLabelValue()); } } - - 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() { + return info; } private static TextEditGroup createEditDescription(ICPPASTCompositeTypeSpecifier classNode) { diff --git a/core/org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/refactoring/MethodContext.java b/core/org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/refactoring/MethodContext.java index 612a888749d..ecc7578a922 100644 --- a/core/org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/refactoring/MethodContext.java +++ b/core/org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/refactoring/MethodContext.java @@ -1,12 +1,12 @@ /******************************************************************************* - * Copyright (c) 2008, 2010 Institute for Software, HSR Hochschule fuer Technik + * Copyright (c) 2008, 2010 Institute for Software, HSR Hochschule fuer Technik * Rapperswil, University of applied sciences and others - * All rights reserved. This program and the accompanying materials - * are made available under the terms of the Eclipse Public License v1.0 - * which accompanies this distribution, and is available at - * http://www.eclipse.org/legal/epl-v10.html - * - * Contributors: + * All rights reserved. This program and the accompanying materials + * are made available under the terms of the Eclipse Public License v1.0 + * which accompanies this distribution, and is available at + * http://www.eclipse.org/legal/epl-v10.html + * + * Contributors: * Institute for Software - initial API and implementation *******************************************************************************/ package org.eclipse.cdt.internal.ui.refactoring; @@ -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 - * determine if methods are in the same class. + * Represents a function or method and adds some useful helper methods to determine + * if two methods are in the same class. */ public class MethodContext { public enum ContextType { NONE, FUNCTION, METHOD } @@ -55,7 +55,7 @@ public class MethodContext { public IASTName getMethodDeclarationName() { return declarationName; } - + public IASTDeclaration getMethodDeclaration() { IASTNode parent = declarationName.getParent().getParent(); if (parent instanceof IASTDeclaration) { @@ -63,7 +63,7 @@ public class MethodContext { } return null; } - + public Visibility getMethodDeclarationVisibility() { return Visibility.getVisibility(declarationName); } @@ -71,17 +71,17 @@ public class MethodContext { public void setMethodQName(ICPPASTQualifiedName qname) { this.qname = qname; } - + public ICPPASTQualifiedName getMethodQName() { return qname; } - + public static boolean isSameClass(ICPPASTQualifiedName qname1, ICPPASTQualifiedName qname2) { ICPPClassType bind1 = getClassBinding(qname1); ICPPClassType bind2 = getClassBinding(qname2); return bind1.equals(bind2); } - + public static boolean isSameOrSubClass(MethodContext context1, MethodContext contextOfSameOrSubclass) { ICPPInternalBinding bind1 = getICPPInternalBinding(context1); ICPPInternalBinding subclassBind = getICPPInternalBinding(contextOfSameOrSubclass); @@ -90,7 +90,7 @@ public class MethodContext { } return isSubclass(bind1,subclassBind); } - + private static boolean isSubclass(ICPPInternalBinding bind1, ICPPInternalBinding subclassBind) { if (subclassBind instanceof ICPPClassType) { ICPPClassType classType = (ICPPClassType) subclassBind; @@ -110,7 +110,7 @@ public class MethodContext { ICPPInternalBinding bind2 = getICPPInternalBinding(context2); return isSameClass(bind1,bind2); } - + private static boolean isSameClass(ICPPBase base, ICPPInternalBinding bind2) { try { IBinding bind1 = base.getBaseClass(); @@ -118,12 +118,12 @@ public class MethodContext { if (scope1 == null) return false; IASTNode node1 = ASTInternal.getPhysicalNodeOfScope(scope1); - + IScope scope2 = bind2.getScope(); if (scope2 == null) return false; IASTNode node2 = ASTInternal.getPhysicalNodeOfScope(scope2); - + if (node1.equals(node2)) { if (bind1 instanceof ICPPInternalBinding) { ICPPInternalBinding bind1int = (ICPPInternalBinding) bind1; @@ -134,30 +134,30 @@ public class MethodContext { return false; } catch (DOMException e) { return false; - } + } } - + private static boolean isSameClass(ICPPInternalBinding bind1, ICPPInternalBinding bind2) { try { IScope scope1 = bind1.getScope(); if (scope1 == null) return false; IASTNode node1 = ASTInternal.getPhysicalNodeOfScope(scope1); - + IScope scope2 = bind2.getScope(); if (scope2 == null) return false; IASTNode node2 = ASTInternal.getPhysicalNodeOfScope(scope2); - + if (node1.equals(node2)) { return bind1.getDefinition().equals(bind2.getDefinition()); } return false; } catch (DOMException e) { return false; - } + } } - + public static ICPPInternalBinding getICPPInternalBinding(MethodContext context) { IASTName decl = context.getMethodDeclarationName(); IASTNode node = decl; @@ -169,17 +169,17 @@ public class MethodContext { } node = node.getParent(); } - + return null; } - + public boolean isInline() { return qname == null; } - + private static ICPPClassType getClassBinding(ICPPASTQualifiedName qname) { IASTName classname = qname.getNames()[qname.getNames().length - 2]; - ICPPClassType bind = (ICPPClassType)classname.resolveBinding(); + ICPPClassType bind = (ICPPClassType)classname.resolveBinding(); return bind; } diff --git a/core/org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/refactoring/extractfunction/ExtractFunctionInformation.java b/core/org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/refactoring/extractfunction/ExtractFunctionInformation.java index 2d3713394a3..427e1f883cb 100644 --- a/core/org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/refactoring/extractfunction/ExtractFunctionInformation.java +++ b/core/org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/refactoring/extractfunction/ExtractFunctionInformation.java @@ -1,12 +1,12 @@ /******************************************************************************* - * Copyright (c) 2008, 2012 Institute for Software, HSR Hochschule fuer Technik + * Copyright (c) 2008, 2012 Institute for Software, HSR Hochschule fuer Technik * Rapperswil, University of applied sciences and others - * All rights reserved. This program and the accompanying materials - * are made available under the terms of the Eclipse Public License v1.0 - * which accompanies this distribution, and is available at - * http://www.eclipse.org/legal/epl-v10.html - * - * Contributors: + * All rights reserved. This program and the accompanying materials + * are made available under the terms of the Eclipse Public License v1.0 + * which accompanies this distribution, and is available at + * http://www.eclipse.org/legal/epl-v10.html + * + * Contributors: * Institute for Software - initial API and implementation * Sergey Prigogin (Google) *******************************************************************************/ @@ -31,15 +31,15 @@ public class ExtractFunctionInformation { private String methodName; private boolean replaceDuplicates; private List parameters; - private NameInformation mandatoryReturnVariable; + private NameInformation mandatoryReturnVariable; private ICPPASTFunctionDeclarator declarator; private MethodContext context; private boolean isExtractExpression; private boolean virtual; /** - * Returns the function declarator of the method / function from were the statements - * are extacted from. + * Returns the function declarator of the method or function from were the statements + * are extracted from. * @return the function declarator or null */ public ICPPASTFunctionDeclarator getDeclarator() { 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 a1ca1ee8f5a..4a1618d5268 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 @@ -1,12 +1,12 @@ /******************************************************************************* - * Copyright (c) 2008, 2012 Institute for Software, HSR Hochschule fuer Technik + * Copyright (c) 2008, 2012 Institute for Software, HSR Hochschule fuer Technik * Rapperswil, University of applied sciences and others - * All rights reserved. This program and the accompanying materials - * are made available under the terms of the Eclipse Public License v1.0 - * which accompanies this distribution, and is available at - * http://www.eclipse.org/legal/epl-v10.html - * - * Contributors: + * All rights reserved. This program and the accompanying materials + * are made available under the terms of the Eclipse Public License v1.0 + * which accompanies this distribution, and is available at + * http://www.eclipse.org/legal/epl-v10.html + * + * Contributors: * Institute for Software - initial API and implementation * Sergey Prigogin (Google) *******************************************************************************/ @@ -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; @@ -172,17 +173,17 @@ public class ExtractFunctionRefactoring extends CRefactoring { ast = getAST(tu, sm.newChild(1)); nodeFactory = ast.getASTNodeFactory(); container = findExtractableNodes(); - + if (isProgressMonitorCanceled(sm, initStatus)) return initStatus; - + if (container.isEmpty()) { initStatus.addFatalError(Messages.ExtractFunctionRefactoring_NoStmtSelected); return initStatus; } - + checkForNonExtractableStatements(container, initStatus); - + List returnValueCandidates = container.getReturnValueCandidates(); if (returnValueCandidates.size() > 1) { initStatus.addFatalError(Messages.ExtractFunctionRefactoring_TooManySelected); @@ -190,27 +191,27 @@ public class ExtractFunctionRefactoring extends CRefactoring { } else if (returnValueCandidates.size() == 1) { info.setMandatoryReturnVariable(returnValueCandidates.get(0)); } - + info.setParameters(container.getParameterCandidates()); initStatus.merge(checkParameterAndReturnTypes()); if (initStatus.hasFatalError()) return initStatus; - + extractor = FunctionExtractor.createFor(container.getNodesToWrite()); - + if (extractor.canChooseReturnValue() && info.getMandatoryReturnVariable() == null) { chooseReturnVariable(); } - + IPreferencesService preferences = Platform.getPreferencesService(); final boolean outFirst = preferences.getBoolean(CUIPlugin.PLUGIN_ID, PreferenceConstants.FUNCTION_OUTPUT_PARAMETERS_BEFORE_INPUT, false, PreferenceConstants.getPreferenceScopes(project.getProject())); info.sortParameters(outFirst); - + boolean isExtractExpression = container.getNodesToWrite().get(0) instanceof IASTExpression; info.setExtractExpression(isExtractExpression); - + info.setDeclarator(getDeclaration(container.getNodesToWrite().get(0))); MethodContext context = NodeHelper.findMethodContext(container.getNodesToWrite().get(0), refactoringContext, sm.newChild(1)); @@ -347,7 +348,7 @@ public class ExtractFunctionRefactoring extends CRefactoring { if (node != firstNodeToWrite) { rewriter.remove(node, editGroup); } - } + } } private void insertCallIntoTree(IASTNode methodCall, List list, ASTRewrite rewriter, @@ -379,9 +380,10 @@ public class ExtractFunctionRefactoring extends CRefactoring { } private void createMethodDefinition(final IASTName methodName, MethodContext context, - IASTNode firstNode, ModificationCollector collector) { - IASTFunctionDefinition node = CPPVisitor.findAncestorWithType(firstNode, IASTFunctionDefinition.class); - if (node != null) { + IASTNode firstExtractedNode, ModificationCollector collector) { + IASTFunctionDefinition functionToExtractFrom = + 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()); - addMethod(methodName, context, rewriter, node, new TextEditGroup(title)); + ASTRewrite rewriter = collector.rewriterForTranslationUnit(functionToExtractFrom.getTranslationUnit()); + 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) { @@ -600,7 +602,7 @@ public class ExtractFunctionRefactoring extends CRefactoring { IASTDeclSpecifier returnType = getReturnType(); func.setDeclSpecifier(returnType); - + IASTStandardFunctionDeclarator createdFunctionDeclarator = extractor.createFunctionDeclarator(qname, info.getDeclarator(), info.getReturnVariable(), container.getNodesToWrite(), @@ -609,22 +611,46 @@ public class ExtractFunctionRefactoring extends CRefactoring { IASTCompoundStatement compound = new CPPASTCompoundStatement(); 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(); templateDeclaration.setParent(ast); - + for (ICPPASTTemplateParameter param : parentTemplate.getTemplateParameters()) { 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(), @@ -777,7 +803,7 @@ public class ExtractFunctionRefactoring extends CRefactoring { IASTNode node = container.getNodesToWrite().get(0); return extractor.createReturnAssignment(node, stmt, callExpression); } - + private IASTSimpleDeclaration getDeclaration(IASTName name) { IASTSimpleDeclaration simpleDecl = new CPPASTSimpleDeclaration(); IASTStandardFunctionDeclarator declarator =