Bugzilla – Full Text Bug Listing
| Summary: | [clean up] Convert to enhanced for-loop on save results in identity comparison | ||
|---|---|---|---|
| Product: | [Eclipse Project] JDT | Reporter: | Steve <steve> |
| Component: | UI | Assignee: | Deepak Azad <deepakazad> |
| Status: | RESOLVED FIXED | QA Contact: | |
| Severity: | normal | ||
| Priority: | P3 | CC: | markus.kell.r, steve |
| Version: | 3.7 | ||
| Target Milestone: | 3.8 M3 | ||
| Hardware: | PC | ||
| OS: | Windows 7 | ||
| Whiteboard: | |||
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 |
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; }