Community
Participate
Working Groups
Editor save action "Remove unnecessary parentheses" makes code uncompilable. Code line below before save: static final Short cache[] = new Short[-(-128) + 127 + 1]; //compilable code line after save: static final Short cache[] = new Short[--128 + 127 + 1]; //uncompilable This line of code is from java.lang.Short.
(In reply to comment #0) > Editor save action "Remove unnecessary parentheses" makes code uncompilable. I guess you mean the following option: Use parentheses in expressions: Only if necessary Can reproduce with 4.3 M6.
Created attachment 229549 [details] Patch NecessaryParenthesesChecker#needsParentheses(..) had not considered the case where both the parent and child expressions are PrefixExpression. In such a case, we can have a pair of prefix operators (+, -, ~, !, ++ and --) separated from each other with opening parentheses. Out of the 36 possible pairs: a) For some, the parentheses can be safely removed. Example: +(-Expression) => +-Expression b) Some are already invalid and have a compilation error, so parentheses can be removed, which is the existing behavior and has not been changed. Example: ++(PrefixOperator Expression) c) Following pairs lead to a compilation error or semantic change on removing parentheses: +(+128) => ++128 (compile error) +(+i) => ++i (semantic change) +(++i) => +++i (considered as ++(+i), compile error and semantic change) -(-128) => --128 (compile error) -(-i) => --i (semantic change) -(--i) => ---i (considered as --(-i), compile error and semantic change) +(++128) and -(--128) are invalid and the child expression comes as a NumberLiteral, hence these do not fall under the case where both parent and child are PrefixExpression. For the above cases in c), the patch contains a check to preserve the parentheses. Also, added corresponding test cases in CleanUpTest. Dani, please check if the behavior is fine.
*** Bug 386028 has been marked as a duplicate of this bug. ***
Thanks Noopur, the patch looks good. Committed patch with http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=c47cfd066ed2625d7d5bcb86255bd873a5058b8b
Note that this fix is not strictly correct. NecessaryParenthesesChecker is meant to handle cases where only parentheses can preserve semantics. But in this case, a whitespace between the two "-" is enough: "- -128". The code formatter also preserves this whitespace (and bug 405038 recently fixed a similar problem for infix expressions). The proper fix would be in the ASTRewrite, see bug 404523.
(In reply to comment #5) > Note that this fix is not strictly correct. > > NecessaryParenthesesChecker is meant to handle cases where only parentheses > can preserve semantics. But in this case, a whitespace between the two "-" > is enough: "- -128". The code formatter also preserves this whitespace (and > bug 405038 recently fixed a similar problem for infix expressions). > > The proper fix would be in the ASTRewrite, see bug 404523. I suggest we keep the workaround until bug 404523 has been fixed.
> I suggest we keep the workaround until bug 404523 has been fixed. I agree. However, to finish this off, we should also handle the same problems with InfixExpressions, e.g.: int i = 1-(-128); AFAICS, PostfixExpressions don't pose a problem, since they bind stronger than Prefix and Infix and they don't exist in unary form. Noopur, could you please have another look?
Created attachment 229785 [details] Patch-updated The same cases apply for infix parent expressions also. The attached patch has the updated code. Dani, please review.
Looks good. Go ahead and commit it.
Thanks Dani. Committed patch with http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=e671df9ce3e004b5f28924e663b4002e4cd9266d
.
N20130421-2000 has a test failure in InlineMethodTests.testPrefixPlus. Noopur, please fix the test if the change makes sense.
Created attachment 229954 [details] Patch-Fixed Test failure (In reply to comment #12) > N20130421-2000 has a test failure in InlineMethodTests.testPrefixPlus. > Noopur, please fix the test if the change makes sense. The test failure is in the following case: public void foo() { int i= 10; result= inline(++i); // After inline operation: // Earlier: result= 1 + ++i; // Now: result= 1 + (++i); } public int inline(int x) { return 1 + x; } As mentioned in comment #5, this fix does not consider if a whitespace is enough to prevent the bug and keeps more parentheses than needed to avoid the bug without a whitespace. It is a workaround for bug 404523. So the test update should also be a workaround until bug 404523 is fixed and this fix is reverted. I have updated the test currently. Markus, please check and let me know if it is fine.
Looks good, please commit and resolve this bug again.
Thanks Markus. Committed with: http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=5523c3f8b72f7756ed30b5b0cf6ab5ec36d8392c
