Attachment 178759 Details for Bug 319069 – First stab

[patch] First stab

patch.txt (text/plain), 13.85 KB, created by Markus Keller

on 2010-09-13 12:36:46 EDT

(hide)

Description:

Filename:

MIME Type:

Creator: Markus Keller

Created: 2010-09-13 12:36:46 EDT

Size: 13.85 KB

patch

obsolete

>### Eclipse Workspace Patch 1.0
>#P org.eclipse.jdt.ui
>Index: core extension/org/eclipse/jdt/internal/corext/dom/VariableDeclarationRewrite.java
>===================================================================
>RCS file: /cvsroot/eclipse/org.eclipse.jdt.ui/core extension/org/eclipse/jdt/internal/corext/dom/VariableDeclarationRewrite.java,v
>retrieving revision 1.7
>diff -u -r1.7 VariableDeclarationRewrite.java
>--- core extension/org/eclipse/jdt/internal/corext/dom/VariableDeclarationRewrite.java	14 May 2009 15:25:58 -0000	1.7
>+++ core extension/org/eclipse/jdt/internal/corext/dom/VariableDeclarationRewrite.java	13 Sep 2010 16:34:27 -0000
>@@ -1,5 +1,5 @@
> /*******************************************************************************
>- * Copyright (c) 2000, 2009 IBM Corporation and others.
>+ * Copyright (c) 2000, 2010 IBM Corporation 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
>@@ -11,8 +11,12 @@
> package org.eclipse.jdt.internal.corext.dom;
> 
> import java.util.Arrays;
>+import java.util.Collections;
>+import java.util.HashMap;
> import java.util.Iterator;
> import java.util.List;
>+import java.util.Map;
>+import java.util.WeakHashMap;
> 
> import org.eclipse.core.runtime.Assert;
> 
>@@ -24,6 +28,7 @@
> import org.eclipse.jdt.core.dom.AnonymousClassDeclaration;
> import org.eclipse.jdt.core.dom.Block;
> import org.eclipse.jdt.core.dom.FieldDeclaration;
>+import org.eclipse.jdt.core.dom.Modifier;
> import org.eclipse.jdt.core.dom.SingleVariableDeclaration;
> import org.eclipse.jdt.core.dom.SwitchStatement;
> import org.eclipse.jdt.core.dom.Type;
>@@ -35,6 +40,12 @@
> 
> 
> public class VariableDeclarationRewrite {
>+	
>+	/**
>+	 * Map from {@link ASTRewrite} to ({@link Map} from {@link VariableDeclarationFragment} to {@link MovedFragment}).
>+	 * @since 3.7
>+	 */
>+	private static Map fgASTRewriteToFieldDeclLookup= Collections.synchronizedMap(new WeakHashMap());
> 
> 	public static void rewriteModifiers(final SingleVariableDeclaration declarationNode, final int includedModifiers, final int excludedModifiers, final ASTRewrite rewrite, final TextEditGroup group) {
> 		ModifierRewrite listRewrite= ModifierRewrite.create(rewrite, declarationNode);
>@@ -49,7 +60,20 @@
> 	public static void rewriteModifiers(final FieldDeclaration declarationNode, final VariableDeclarationFragment[] toChange, final int includedModifiers, final int excludedModifiers, final ASTRewrite rewrite, final TextEditGroup group) {
> 		final List fragmentsToChange= Arrays.asList(toChange);
> 		AST ast= declarationNode.getAST();
>-
>+/*
>+ * Problem: Same declarationNode can be the subject of multiple calls to this method.
>+ * For the 2nd++ calls, the original declarationNode has already been rewritten, and this has to be taken into account.
>+ * 
>+ * Assumption:
>+ * - modifiers for each VariableDeclarationFragment are modified at most once
>+ * 
>+ * Solution:
>+ * - Maintain a map from original VariableDeclarationFragments to their new FieldDeclaration.
>+ * - Original modifiers in declarationNode belong to the first fragment.
>+ * - When a later fragment needs different modifiers, we create a new FieldDeclaration and move all successive fragments into that declaration
>+ * - When a fragment has been moved to a new declaration, make sure we don't create a new move target again, but instead use the already created one 
>+ */
>+//TODO: Leak Test. => Problem: WeakHashMap only releases recoverable values on some method calls (on demand). => Temporary leaks
> 		List fragments= declarationNode.fragments();
> 		Iterator iter= fragments.iterator();
> 
>@@ -63,41 +87,96 @@
> 		VariableDeclarationFragment lastFragment= (VariableDeclarationFragment)iter.next();
> 		ASTNode lastStatement= declarationNode;
> 
>-		boolean modifiersModified= false;
> 		if (fragmentsToChange.contains(lastFragment)) {
> 			ModifierRewrite modifierRewrite= ModifierRewrite.create(rewrite, declarationNode);
> 			modifierRewrite.setModifiers(includedModifiers, excludedModifiers, group);
>-			modifiersModified= true;
> 		}
> 
> 		ListRewrite fragmentsRewrite= null;
> 		while (iter.hasNext()) {
> 			VariableDeclarationFragment currentFragment= (VariableDeclarationFragment)iter.next();
> 
>-			if (fragmentsToChange.contains(lastFragment) != fragmentsToChange.contains(currentFragment)) {
>-
>-					FieldDeclaration newStatement= ast.newFieldDeclaration((VariableDeclarationFragment)rewrite.createMoveTarget(currentFragment));
>-					newStatement.setType((Type)rewrite.createCopyTarget(declarationNode.getType()));
>+			Map lookup= getLookup(rewrite);
>+			MovedFragment currentMovedFragment= (MovedFragment)lookup.get(currentFragment);
>+			
>+			boolean changeLast= fragmentsToChange.contains(lastFragment);
>+			boolean changeCurrent= fragmentsToChange.contains(currentFragment);
>+			if (changeLast != changeCurrent || lookup.containsKey(lastFragment)) {
>+				if (currentMovedFragment != null) {
>+					// Current fragment has already been moved. Need to put in the right modifiers (removing any exisiting ones).
>+					ModifierRewrite modifierRewrite= ModifierRewrite.create(rewrite, currentMovedFragment.fDeclaration);
>+					ListRewrite listRewrite= rewrite.getListRewrite(currentMovedFragment.fDeclaration, FieldDeclaration.MODIFIERS2_PROPERTY);
>+					List extendedList= listRewrite.getRewrittenList();
>+					for (int i= 0; i < extendedList.size(); i++) {
>+						ASTNode curr= (ASTNode)extendedList.get(i);
>+						if (curr instanceof Modifier)
>+							rewrite.remove(curr, group);
>+					}
>+					if (changeCurrent) {
>+						int newModifiers= (declarationNode.getModifiers() & ~excludedModifiers) | includedModifiers;
>+						modifierRewrite.setModifiers(newModifiers, excludedModifiers, group);
>+					} else {
>+						int newModifiers= declarationNode.getModifiers();
>+						modifierRewrite.setModifiers(newModifiers, Modifier.NONE, group);
>+					}
>+					
>+				} else { // need to split an existing field declaration
>+					VariableDeclarationFragment moveTarget;
>+					moveTarget= (VariableDeclarationFragment)rewrite.createMoveTarget(currentFragment);
>+					
>+					FieldDeclaration newStatement= (FieldDeclaration)ast.createInstance(FieldDeclaration.class);
>+					rewrite.getListRewrite(newStatement, FieldDeclaration.FRAGMENTS_PROPERTY).insertLast(moveTarget, group);
>+					lookup.put(currentFragment, new MovedFragment(moveTarget, newStatement));
>+					rewrite.set(newStatement, FieldDeclaration.TYPE_PROPERTY, rewrite.createCopyTarget(declarationNode.getType()), group);
> 
> 					ModifierRewrite modifierRewrite= ModifierRewrite.create(rewrite, newStatement);
>-					if (fragmentsToChange.contains(currentFragment)) {
>-						modifierRewrite.copyAllAnnotations(declarationNode, group);
>+					modifierRewrite.copyAllAnnotations(declarationNode, group);
>+					if (changeCurrent) {
> 						int newModifiers= (declarationNode.getModifiers() & ~excludedModifiers) | includedModifiers;
> 						modifierRewrite.setModifiers(newModifiers, excludedModifiers, group);
> 					} else {
>-						modifierRewrite.copyAllModifiers(declarationNode, group, modifiersModified);
>+						int newModifiers= declarationNode.getModifiers();
>+						modifierRewrite.setModifiers(newModifiers, Modifier.NONE, group);
> 					}
> 					blockRewrite.insertAfter(newStatement, lastStatement, group);
> 
> 					fragmentsRewrite= rewrite.getListRewrite(newStatement, FieldDeclaration.FRAGMENTS_PROPERTY);
> 					lastStatement= newStatement;
>+				}
>+					
> 			} else if (fragmentsRewrite != null) {
>-				ASTNode fragment0= rewrite.createMoveTarget(currentFragment);
>+				VariableDeclarationFragment fragment0;
>+				if (currentMovedFragment != null) {
>+					fragment0= currentMovedFragment.fMoveTarget;
>+					rewrite.getListRewrite(currentMovedFragment.fDeclaration, FieldDeclaration.FRAGMENTS_PROPERTY).remove(fragment0, group);
>+				} else {
>+					fragment0= (VariableDeclarationFragment)rewrite.createMoveTarget(currentFragment);
>+				}
>+				lookup.put(currentFragment, new MovedFragment(fragment0, lastStatement));
> 				fragmentsRewrite.insertLast(fragment0, group);
> 			}
> 			lastFragment= currentFragment;
> 		}
> 	}
>+	
>+	private static class MovedFragment {
>+		final VariableDeclarationFragment fMoveTarget;
>+		final ASTNode fDeclaration;
>+		
>+		public MovedFragment(VariableDeclarationFragment moveTarget, ASTNode declaration) {
>+			fMoveTarget= moveTarget;
>+			fDeclaration= declaration;
>+		}
>+	}
>+
>+	private static Map/*<VariableDeclarationFragment, MovedFragment>*/ getLookup(ASTRewrite rewrite) {
>+		Map lookup= (Map) fgASTRewriteToFieldDeclLookup.get(rewrite);
>+		if (lookup == null) {
>+			lookup= new HashMap();
>+			fgASTRewriteToFieldDeclLookup.put(rewrite, lookup);
>+		}
>+		return lookup;
>+	}
> 
> 	public static void rewriteModifiers(final VariableDeclarationStatement declarationNode, final VariableDeclarationFragment[] toChange, final int includedModifiers, final int excludedModifiers, ASTRewrite rewrite, final TextEditGroup group) {
> 		final List fragmentsToChange= Arrays.asList(toChange);
>Index: core refactoring/org/eclipse/jdt/internal/corext/refactoring/structure/MemberVisibilityAdjustor.java
>===================================================================
>RCS file: /cvsroot/eclipse/org.eclipse.jdt.ui/core refactoring/org/eclipse/jdt/internal/corext/refactoring/structure/MemberVisibilityAdjustor.java,v
>retrieving revision 1.47
>diff -u -r1.47 MemberVisibilityAdjustor.java
>--- core refactoring/org/eclipse/jdt/internal/corext/refactoring/structure/MemberVisibilityAdjustor.java	5 Mar 2010 17:36:50 -0000	1.47
>+++ core refactoring/org/eclipse/jdt/internal/corext/refactoring/structure/MemberVisibilityAdjustor.java	13 Sep 2010 16:34:27 -0000
>@@ -12,6 +12,7 @@
> 
> import java.util.HashMap;
> import java.util.Iterator;
>+import java.util.LinkedHashMap;
> import java.util.Map;
> 
> import org.eclipse.core.runtime.Assert;
>@@ -38,25 +39,20 @@
> import org.eclipse.jdt.core.JavaCore;
> import org.eclipse.jdt.core.JavaModelException;
> import org.eclipse.jdt.core.WorkingCopyOwner;
>-import org.eclipse.jdt.core.dom.ASTNode;
>-import org.eclipse.jdt.core.dom.AbstractTypeDeclaration;
> import org.eclipse.jdt.core.dom.BodyDeclaration;
> import org.eclipse.jdt.core.dom.CompilationUnit;
> import org.eclipse.jdt.core.dom.FieldDeclaration;
>-import org.eclipse.jdt.core.dom.IExtendedModifier;
> import org.eclipse.jdt.core.dom.Modifier;
>-import org.eclipse.jdt.core.dom.SimpleName;
>-import org.eclipse.jdt.core.dom.Type;
>-import org.eclipse.jdt.core.dom.VariableDeclarationFragment;
> import org.eclipse.jdt.core.dom.Modifier.ModifierKeyword;
>+import org.eclipse.jdt.core.dom.VariableDeclarationFragment;
> import org.eclipse.jdt.core.dom.rewrite.ASTRewrite;
>-import org.eclipse.jdt.core.dom.rewrite.ListRewrite;
> import org.eclipse.jdt.core.search.IJavaSearchConstants;
> import org.eclipse.jdt.core.search.IJavaSearchScope;
> import org.eclipse.jdt.core.search.SearchMatch;
> import org.eclipse.jdt.core.search.SearchPattern;
> 
> import org.eclipse.jdt.internal.corext.dom.ModifierRewrite;
>+import org.eclipse.jdt.internal.corext.dom.VariableDeclarationRewrite;
> import org.eclipse.jdt.internal.corext.refactoring.RefactoringCoreMessages;
> import org.eclipse.jdt.internal.corext.refactoring.RefactoringScopeFactory;
> import org.eclipse.jdt.internal.corext.refactoring.RefactoringSearchEngine2;
>@@ -168,32 +164,8 @@
> 			if (fMember instanceof IField && !Flags.isEnum(fMember.getFlags())) {
> 				final VariableDeclarationFragment fragment= ASTNodeSearchUtil.getFieldDeclarationFragmentNode((IField) fMember, root);
> 				final FieldDeclaration declaration= (FieldDeclaration) fragment.getParent();
>-				if (declaration.fragments().size() == 1)
>-					ModifierRewrite.create(rewrite, declaration).setVisibility(visibility, group);
>-				else {
>-					final VariableDeclarationFragment newFragment= rewrite.getAST().newVariableDeclarationFragment();
>-					newFragment.setName((SimpleName) rewrite.createCopyTarget(fragment.getName()));
>-					final FieldDeclaration newDeclaration= rewrite.getAST().newFieldDeclaration(newFragment);
>-					newDeclaration.setType((Type) rewrite.createCopyTarget(declaration.getType()));
>-					IExtendedModifier extended= null;
>-					for (final Iterator iterator= declaration.modifiers().iterator(); iterator.hasNext();) {
>-						extended= (IExtendedModifier) iterator.next();
>-						if (extended.isModifier()) {
>-							final Modifier modifier= (Modifier) extended;
>-							final int flag= modifier.getKeyword().toFlagValue();
>-							if ((flag & (Modifier.PUBLIC | Modifier.PROTECTED | Modifier.PRIVATE)) != 0)
>-								continue;
>-						}
>-						newDeclaration.modifiers().add(rewrite.createCopyTarget((ASTNode) extended));
>-					}
>-					ModifierRewrite.create(rewrite, newDeclaration).setVisibility(visibility, group);
>-					final AbstractTypeDeclaration type= (AbstractTypeDeclaration) declaration.getParent();
>-					rewrite.getListRewrite(type, type.getBodyDeclarationsProperty()).insertAfter(newDeclaration, declaration, null);
>-					final ListRewrite list= rewrite.getListRewrite(declaration, FieldDeclaration.FRAGMENTS_PROPERTY);
>-					list.remove(fragment, group);
>-					if (list.getRewrittenList().isEmpty())
>-						rewrite.remove(declaration, null);
>-				}
>+				VariableDeclarationFragment[] fragmentsToChange= new VariableDeclarationFragment[] { fragment };
>+				VariableDeclarationRewrite.rewriteModifiers(declaration, fragmentsToChange, visibility, ModifierRewrite.VISIBILITY_MODIFIERS, rewrite, group);
> 				if (status != null)
> 					adjustor.fStatus.merge(status);
> 			} else if (fMember != null) {
>@@ -448,7 +420,7 @@
> 	}
> 
> 	/** The map of members to visibility adjustments */
>-	private Map fAdjustments= new HashMap();
>+	private Map fAdjustments= new LinkedHashMap(); // LinkedHashMap to preserve order of generated warnings
> 
> 	/** Should incoming references be adjusted? */
> 	private boolean fIncoming= true;

Actions: View | Diff

Attachments on bug 319069: 178759 | 180499 | 180878