314365: reliable rewriting

2 files changed, 51 insertions, 10 deletions

diff --git a/org.aspectj.matcher/src/org/aspectj/weaver/patterns/PointcutEvaluationExpenseComparator.java b/org.aspectj.matcher/src/org/aspectj/weaver/patterns/PointcutEvaluationExpenseComparator.java
index 2fb9321f9..1d2643261 100644
--- a/org.aspectj.matcher/src/org/aspectj/weaver/patterns/PointcutEvaluationExpenseComparator.java
+++ b/org.aspectj.matcher/src/org/aspectj/weaver/patterns/PointcutEvaluationExpenseComparator.java
@@ -47,7 +47,6 @@ public class PointcutEvaluationExpenseComparator implements Comparator<Pointcut>
 	 * @args cflow, cflowbelow if
 	 */
 	public int compare(Pointcut p1, Pointcut p2) {
-
 		// important property for a well-defined comparator
 		if (p1.equals(p2)) {
 			return 0;
@@ -56,9 +55,14 @@ public class PointcutEvaluationExpenseComparator implements Comparator<Pointcut>
 		if (result == 0) {
 			// they have the same evaluation expense, but are not 'equal'
 			// sort by hashCode
-			result = p1.hashCode() - p2.hashCode();
-			if (result == 0) {
-				/* not allowed if ne */return -1;
+			int p1code = p1.hashCode();
+			int p2code = p2.hashCode();
+			if (p1code == p2code) {
+				return 0;
+			} else if (p1code < p2code) {
+				return -1;
+			} else {
+				return +1;
 			}
 		}
 		return result;
@@ -129,10 +133,22 @@ public class PointcutEvaluationExpenseComparator implements Comparator<Pointcut>
 			return getScore(((NotPointcut) p).getNegatedPointcut());
 		}
 		if (p instanceof AndPointcut) {
-			return getScore(((AndPointcut) p).getLeft());
+			int leftScore = getScore(((AndPointcut) p).getLeft());
+			int rightScore = getScore(((AndPointcut) p).getRight());
+			if (leftScore < rightScore) {
+				return leftScore;
+			} else {
+				return rightScore;
+			}
 		}
 		if (p instanceof OrPointcut) {
-			return getScore(((OrPointcut) p).getLeft());
+			int leftScore = getScore(((OrPointcut) p).getLeft());
+			int rightScore = getScore(((OrPointcut) p).getRight());
+			if (leftScore > rightScore) {
+				return leftScore;
+			} else {
+				return rightScore;
+			}
 		}
 		return OTHER;
 	}
diff --git a/org.aspectj.matcher/testsrc/org/aspectj/weaver/patterns/PointcutRewriterTest.java b/org.aspectj.matcher/testsrc/org/aspectj/weaver/patterns/PointcutRewriterTest.java
index 60b75b937..d2a8fd245 100644
--- a/org.aspectj.matcher/testsrc/org/aspectj/weaver/patterns/PointcutRewriterTest.java
+++ b/org.aspectj.matcher/testsrc/org/aspectj/weaver/patterns/PointcutRewriterTest.java
@@ -18,15 +18,39 @@ import junit.framework.TestCase;
 import org.aspectj.weaver.Shadow;
 
 /**
- * @author colyer
+ * Testing the pointcut rewriter.
  * 
- *         TODO To change the template for this generated type comment go to Window - Preferences - Java - Code Style - Code
- *         Templates
+ * @author Adrian Colyer
+ * @author Andy Clement
  */
 public class PointcutRewriterTest extends TestCase {
 
 	private PointcutRewriter prw;
 
+	public void testComplexRewrite1() {
+		Pointcut p = getPointcut("(persingleton(org.eclipse.ajdt.internal.ui.ras.UIFFDC) && ((handler(java.lang.Throwable+) && args(arg1)) && ((within(org.eclipse.ajdt..*) && (!within(org.eclipse.ajdt.internal.ui.lazystart..*) && (!within(org.eclipse.ajdt.internal.ui.dialogs.OpenTypeSelectionDialog2) && !(within(org.eclipse.ajdt.internal.ui.editor.AspectJBreakpointRulerAction) && handler(org.eclipse.jface.text.BadLocationException))))) && (!(within(org.eclipse.ajdt.core.ras.FFDC+) || handler(org.eclipse.core.runtime.OperationCanceledException)) && !this(java.lang.Object)))))");
+		checkMultipleRewrite(p);
+		p = getPointcut("((((((((((!within(org.eclipse.ajdt.internal.ui.lazystart..*) && !within(org.eclipse.ajdt.internal.ui.dialogs.OpenTypeSelectionDialog2)) && !within(org.eclipse.ajdt.core.ras.FFDC+)) && within(org.eclipse.ajdt..*)) && !within(org.eclipse.ajdt.internal.ui.editor.AspectJBreakpointRulerAction)) && handler(java.lang.Throwable+)) && !handler(org.eclipse.core.runtime.OperationCanceledException)) && !this(java.lang.Object)) && args(arg1)) && persingleton(org.eclipse.ajdt.internal.ui.ras.UIFFDC)) || (((((((((!within(org.eclipse.ajdt.internal.ui.lazystart..*) && !within(org.eclipse.ajdt.internal.ui.dialogs.OpenTypeSelectionDialog2)) && !within(org.eclipse.ajdt.core.ras.FFDC+)) && within(org.eclipse.ajdt..*)) && !handler(org.eclipse.jface.text.BadLocationException)) && handler(java.lang.Throwable+)) && !handler(org.eclipse.core.runtime.OperationCanceledException)) && !this(java.lang.Object)) && args(arg1)) && persingleton(org.eclipse.ajdt.internal.ui.ras.UIFFDC)))");
+		checkMultipleRewrite(p);
+		p = getPointcut("(persingleton(Oranges) && ((handler(Apples+) && args(arg1)) && ((within(foo..*) && (!within(org.eclipse.ajdt.internal.ui.lazystart..*) && (!within(org.eclipse.ajdt.internal.ui.dialogs.OpenTypeSelectionDialog2) && !(within(org.eclipse.ajdt.internal.ui.editor.AspectJBreakpointRulerAction) && handler(org.eclipse.jface.text.BadLocationException))))) && (!(within(org.eclipse.ajdt.core.ras.FFDC+) || handler(org.eclipse.core.runtime.OperationCanceledException)) && !this(java.lang.Object)))))");
+		checkMultipleRewrite(p);
+		p = getPointcut("(((handler(Apples+)) && ((within(foo..*) && (!within(org.eclipse.ajdt.internal.ui.lazystart..*) && (!within(org.eclipse.ajdt.internal.ui.dialogs.OpenTypeSelectionDialog2) && !(within(org.eclipse.ajdt.internal.ui.editor.AspectJBreakpointRulerAction) && handler(org.eclipse.jface.text.BadLocationException))))) && (!(within(org.eclipse.ajdt.core.ras.FFDC+) || handler(org.eclipse.core.runtime.OperationCanceledException)) && !this(java.lang.Object)))))");
+		checkMultipleRewrite(p);
+		p = getPointcut("within(xxx..*) && within(XXY) && within(org.eclipse.AspectJBreakpoint)");
+		checkMultipleRewrite(p);
+	}
+
+	/**
+	 * Rewrites a pointcut twice and checks the format is stable
+	 */
+	private void checkMultipleRewrite(Pointcut p) {
+		Pointcut rewrittenPointcut = prw.rewrite(p, false);
+		String rewrite = rewrittenPointcut.toString();
+		Pointcut rewriteOfRewrittenPointcut = prw.rewrite(rewrittenPointcut, false);
+		String rewriteOfRewritten = rewriteOfRewrittenPointcut.toString();
+		assertEquals(rewrite, rewriteOfRewritten);
+	}
+
 	public void testDistributeNot() {
 		Pointcut plain = getPointcut("this(Foo)");
 		assertEquals("Unchanged", plain, prw.rewrite(plain));
@@ -417,9 +441,10 @@ public class PointcutRewriterTest extends TestCase {
 
 	public void testOrderingInAnd() {
 		Pointcut bigLongPC = getPointcut("cflow(this(Foo)) && @args(X) && args(X) && @this(Foo) && @target(Boo) && this(Moo) && target(Boo) && @annotation(Moo) && @withincode(Boo) && withincode(new(..)) && set(* *)&& @within(Foo) && within(Foo)");
+		checkMultipleRewrite(bigLongPC);
 		Pointcut rewritten = prw.rewrite(bigLongPC);
 		assertEquals(
-				"((((((((((((within(Foo) && @within(Foo)) && set(* *)) && withincode(new(..))) && @withincode(Boo)) && target(Boo)) && this(Moo)) && @annotation(Moo)) && @target(Boo)) && @this(Foo)) && args(X)) && @args(X)) && cflow(this(Foo)))",
+				"((((((((((((within(Foo) && @within(Foo)) && set(* *)) && withincode(new(..))) && @withincode(Boo)) && target(Boo)) && this(Moo)) && @annotation(Moo)) && @this(Foo)) && @target(Boo)) && args(X)) && @args(X)) && cflow(this(Foo)))",
 				rewritten.toString());
 	}