Community
Participate
Working Groups
Build Identifier: 20100917-0705 See steps to reproduce. This means this refactoring on save must be disabled otherwise bugs to our code can easily sneak in. Reproducible: Always Steps to Reproduce: Save this code (part of a compareTo method): for (int i = 0; i < tokens.length; i++) { // A comment int v = Utils.compare(tokens[i], ((MyClass) obj).tokens[i]); if (v!=0) return v; } After saving, convert on save produces this: for (String token : tokens) { // Comment int v = Utils.compare(token, token); if (v!=0) return v; }
Apologies if this isn't a JDI UI area - I chose this because that's where the per-project preferences are saved.
Deepak, can you please have a look? We should disable the clean up / quick assist when the reference has a qualifier (except for "this." for a field).
(In reply to comment #2) > Deepak, can you please have a look? We should disable the clean up / quick > assist when the reference has a qualifier (except for "this." for a field). I disabled the quick assist when the qualifiers are different in the loop condition and in the loop body. Fixed in HEAD. Commit - 25d38709407efcb36807e640bb076258ccdf5ecc
> Fixed in HEAD. Commit - 25d38709407efcb36807e640bb076258ccdf5ecc With Git, that should be "Fixed in master". Reopening, since there's still a case where the quick assist replaces too much (this.tokens[i] should not be replaced): package test; public class EnhancedForLoopTest implements Comparable<EnhancedForLoopTest> { String tokens[]; @Override public int compareTo(EnhancedForLoopTest obj) { for (int i = 0; i < obj.tokens.length; i++) { // A comment int v = String.CASE_INSENSITIVE_ORDER.compare(this.tokens[i], obj.tokens[i]); if (v != 0) return v; } return 0; } } Furthermore, the quick assist should actually not be disabled for comment 0. It should just not replace "((MyClass) obj).tokens[i]", because that expression is not an access to the loop array.
(In reply to comment #4) > Furthermore, the quick assist should actually not be disabled for comment 0. It > should just not replace "((MyClass) obj).tokens[i]", because that expression is > not an access to the loop array. You mean the result of the conversion for comment 0 should be the following ? (What is 'i' here ?) for (String token : tokens) { // Comment int v = Utils.compare(token, ((MyClass) obj).tokens[i])); if (v!=0) return v; }
(In reply to comment #4) > Reopening, since there's still a case where the quick assist replaces too much > (this.tokens[i] should not be replaced): Good catch! I have disabled the quick assist for this case too. Fixed in master. Commit - 145ad0bbd9ea6dd73eafcaede83f7650591f5012
(In reply to comment #5) > (What is 'i' here ?) D'oh, you're right, that doesn't make sense. But as a last thing, you should use JdtASTMatcher (which also compares bindings) and not ASTMatcher. Counter-example: package test; public class A { String[] things; A other; private A get(A a) { return a; } public void foo(A arg) { for (int i = 0; i < get(other).things.length; i++) { A other= this; // local var shadows field System.out.println(get(other).things[i]); } } }
(In reply to comment #7) > But as a last thing, you should use JdtASTMatcher (which also compares > bindings) and not ASTMatcher. Counter-example: Done. Commit - c8bba345624d3d7962a93343c61b5498cd1b6862
