Attachment 243615 Details for Bug 271526 – patch to review, including Markus's comments

[patch] patch to review, including Markus's comments

271526-2.patch (text/plain), 6.55 KB, created by Jerome Cambon

on 2014-05-28 06:16:48 EDT

(hide)

Description:

Filename:

MIME Type:

Creator: Jerome Cambon

Created: 2014-05-28 06:16:48 EDT

Size: 6.55 KB

patch

obsolete

>diff --git a/org.eclipse.jdt.ui/core refactoring/org/eclipse/jdt/internal/corext/refactoring/code/ConstantChecks.java b/org.eclipse.jdt.ui/core refactoring/org/eclipse/jdt/internal/corext/refactoring/code/ConstantChecks.java
>index 95fff9a..191ff71 100644
>--- a/org.eclipse.jdt.ui/core refactoring/org/eclipse/jdt/internal/corext/refactoring/code/ConstantChecks.java	
>+++ b/org.eclipse.jdt.ui/core refactoring/org/eclipse/jdt/internal/corext/refactoring/code/ConstantChecks.java	
>@@ -7,25 +7,35 @@
>  *
>  * Contributors:
>  *     IBM Corporation - initial API and implementation
>+ *     Jerome Cambon <jerome.cambon@oracle.com> - [extract constant] blocked for anonymous and lambdas that declare variables - https://bugs.eclipse.org/271526
>  *******************************************************************************/
> package org.eclipse.jdt.internal.corext.refactoring.code;
> 
>+import java.util.ArrayList;
>+import java.util.List;
>+
> import org.eclipse.core.runtime.Assert;
> 
> import org.eclipse.jdt.core.dom.ASTVisitor;
>+import org.eclipse.jdt.core.dom.AnonymousClassDeclaration;
> import org.eclipse.jdt.core.dom.FieldAccess;
>+import org.eclipse.jdt.core.dom.FieldDeclaration;
> import org.eclipse.jdt.core.dom.IBinding;
> import org.eclipse.jdt.core.dom.IMethodBinding;
> import org.eclipse.jdt.core.dom.ITypeBinding;
> import org.eclipse.jdt.core.dom.IVariableBinding;
>+import org.eclipse.jdt.core.dom.LambdaExpression;
>+import org.eclipse.jdt.core.dom.MethodDeclaration;
> import org.eclipse.jdt.core.dom.MethodInvocation;
> import org.eclipse.jdt.core.dom.Modifier;
> import org.eclipse.jdt.core.dom.Name;
> import org.eclipse.jdt.core.dom.QualifiedName;
> import org.eclipse.jdt.core.dom.SimpleName;
>+import org.eclipse.jdt.core.dom.SingleVariableDeclaration;
> import org.eclipse.jdt.core.dom.SuperFieldAccess;
> import org.eclipse.jdt.core.dom.SuperMethodInvocation;
> import org.eclipse.jdt.core.dom.ThisExpression;
>+import org.eclipse.jdt.core.dom.VariableDeclarationFragment;
> 
> import org.eclipse.jdt.internal.corext.dom.fragments.ASTFragmentFactory;
> import org.eclipse.jdt.internal.corext.dom.fragments.IExpressionFragment;
>@@ -35,10 +45,18 @@ class ConstantChecks {
> 
> 		private final IExpressionFragment fExpression;
> 		protected boolean fResult= true;
>-
>+		protected boolean fAnonymous= false;
>+		protected boolean fLambda= false;
>+		protected List<String> fLambdaAnonymousNames= new ArrayList<String>();
>+        
> 		public ExpressionChecker(IExpressionFragment ex) {
> 			fExpression= ex;
> 		}
>+
>+		public ExpressionChecker(IExpressionFragment ex, List<String> lambdaAnonymousNames) {
>+			fExpression= ex;
>+			fLambdaAnonymousNames= lambdaAnonymousNames;
>+		}
> 		public boolean check() {
> 			fResult= true;
> 			fExpression.getAssociatedNode().accept(this);
>@@ -50,6 +68,9 @@ class ConstantChecks {
> 		public LoadTimeConstantChecker(IExpressionFragment ex) {
> 			super(ex);
> 		}
>+		public LoadTimeConstantChecker(IExpressionFragment ex, List<String> lambdaAnonymousNames) {
>+			super(ex, lambdaAnonymousNames);
>+		}
> 
> 		@Override
> 		public boolean visit(SuperFieldAccess node) {
>@@ -67,18 +88,54 @@ class ConstantChecks {
> 			return false;
> 		}
> 		@Override
>+		public boolean visit(FieldDeclaration node) {
>+			// Is it really needed ? Seems one never jump into this for anonymous/lambda
>+			if (isAnonymousOrLambda()) {
>+				for (Object fragment : node.fragments()) {
>+					Assert.isTrue(fragment instanceof VariableDeclarationFragment);
>+					fLambdaAnonymousNames.add(((VariableDeclarationFragment) fragment).getName().toString());
>+				}
>+				return true;
>+			}
>+			return true;
>+		}
>+
>+		@Override
> 		public boolean visit(FieldAccess node) {
>-			fResult&= new LoadTimeConstantChecker((IExpressionFragment) ASTFragmentFactory.createFragmentForFullSubtree(node.getExpression())).check();
>+			fResult&= new LoadTimeConstantChecker((IExpressionFragment) ASTFragmentFactory.createFragmentForFullSubtree(node.getExpression()), fLambdaAnonymousNames).check();
> 			return false;
> 		}
> 		@Override
>+		public boolean visit(VariableDeclarationFragment node) {
>+			if (isAnonymousOrLambda()) {
>+				fLambdaAnonymousNames.add(node.getName().toString());
>+			}
>+			return true;
>+		}
>+		@Override
>+		public boolean visit(MethodDeclaration node) {
>+			if (isAnonymousOrLambda()) {
>+				fLambdaAnonymousNames.add(node.getName().toString());
>+				for (Object param : node.parameters()) {
>+					Assert.isTrue(param instanceof SingleVariableDeclaration);
>+					SingleVariableDeclaration varDeclaration= (SingleVariableDeclaration) param;
>+					fLambdaAnonymousNames.add(varDeclaration.getName().toString());
>+				}
>+			}
>+			return true;
>+		}
>+		@Override
> 		public boolean visit(MethodInvocation node) {
>-			if(node.getExpression() == null) {
>+			if (node.getExpression() == null) {
> 				visitName(node.getName());
> 			} else {
>-				fResult&= new LoadTimeConstantChecker((IExpressionFragment) ASTFragmentFactory.createFragmentForFullSubtree(node.getExpression())).check();
>+				if (node.getExpression().resolveTypeBinding().isAnonymous()) {
>+					// If the expression is anonymous, this method invocation cannot be a load time constant
>+					fResult= false;
>+				} else {
>+					fResult&= new LoadTimeConstantChecker((IExpressionFragment) ASTFragmentFactory.createFragmentForFullSubtree(node.getExpression()), fLambdaAnonymousNames).check();
>+				}
> 			}
>-
> 			return false;
> 		}
> 		@Override
>@@ -89,6 +146,23 @@ class ConstantChecks {
> 		public boolean visit(SimpleName node) {
> 			return visitName(node);
> 		}
>+		@Override
>+		public boolean visit(AnonymousClassDeclaration node) {
>+			fAnonymous= true;
>+			return true;
>+		}
>+		@Override
>+		public boolean visit(LambdaExpression node) {
>+			fLambda= true;
>+			return true;
>+		}
>+
>+		private boolean isAnonymousOrLambda() {
>+			if (fAnonymous || fLambda) {
>+				return true;
>+			}
>+			return false;
>+		}
> 
> 		private boolean visitName(Name name) {
> 			fResult&= checkName(name);
>@@ -102,11 +176,15 @@ class ConstantChecks {
> 				                  scenarios which may have been deemed unacceptable in
> 				                  the presence of semantic information will be admitted. */
> 
>+			// Check if the name is declared in anonymous/lambda
>+			if (fLambdaAnonymousNames.contains(name.toString()))
>+				return true;
>+
> 			// If name represents a member:
> 			if (binding instanceof IVariableBinding || binding instanceof IMethodBinding)
> 				return isMemberReferenceValidInClassInitialization(name);
> 			else if (binding instanceof ITypeBinding)
>-				return ! ((ITypeBinding) binding).isTypeVariable();
>+				return !((ITypeBinding) binding).isTypeVariable();
> 			else {
> 				return true; // e.g. a NameQualifiedType's qualifier, which can be a package binding
> 			}

Flags:

markus.kell.r: review-

Actions: View | Diff

Attachments on bug 271526: 243481 | 243615 | 247141