Bug 207754 - [AST][DOM] source range of ParenthesizedExpression does not include the parenthesis
Summary: [AST][DOM] source range of ParenthesizedExpression does not include the paren...
Status: VERIFIED FIXED
Alias: None
Product: JDT
Classification: Eclipse Project
Component: Core (show other bugs)
Version: 3.3   Edit
Hardware: PC Windows XP
: P3 major (vote)
Target Milestone: 3.4 M4   Edit
Assignee: Frederic Fusier CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2007-10-29 07:05 EDT by Benno Baumgartner CLA
Modified: 2008-08-11 03:16 EDT (History)
5 users (show)

See Also:

Attachments
test case for a) (2.71 KB, patch)
2007-10-29 07:18 EDT, Benno Baumgartner CLA 		no flags Details | Diff
Proposed fix (1.27 KB, patch)
2007-10-29 11:25 EDT, Olivier Thomann CLA 		no flags Details | Diff
Regression test (2.35 KB, patch)
2007-10-29 11:26 EDT, Olivier Thomann CLA 		no flags Details | Diff
Proposed patch (8.83 KB, patch)
2007-10-31 12:51 EDT, Frederic Fusier CLA 		no flags Details | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Benno Baumgartner CLA 2007-10-29 07:05:36 EDT 
I20071029-0010

Fup of bug 207332

Given:
package test1;
public class E1 {
	protected String foo(String string) {
		return ("" + string + "") + ("");
	}   
}
Several test cases:

a) 
1. Run clean up with option 'remove extra parenthesis'
Is:
return ("" + string + "") + "";
Should:
return "" + string + "" + ""; 

b) 
1. Select '("" + string + "")'
2. Ctrl-1
3. Remove extra parenthesis
Is:
return ("" + string + "") + ("");
Should:
return "" + string + "" + ("");

c)
1. Set caret at + between the two parenthesized expressions
2. Ctrl-1
3. Exchange left and right operant
Is:
 return ("" + "" + string + "";
Should: 
 return ("") + ("" + string + "");

I've verified my code. In test case b I call (with pExpr=("" + string + ""))

ASTNode moveTarget= rewrite.createMoveTarget(pExpr.getExpression());
rewrite.replace(pExpr, moveTarget, null);

This results in following text edit:
[{MultiTextEdit} [85,16]
  {MoveTargetEdit} [85,0]
  {DeleteEdit} [85,16]
    {MoveSourceEdit} [85,16]]

But I guess MoveSourceEdit should be [86,14] ?
Comment 1 Benno Baumgartner CLA 2007-10-29 07:18:24 EDT 
Created attachment 81436 [details]
test case for a)
Comment 2 Martin Aeschlimann CLA 2007-10-29 07:34:34 EDT 
Looks like a AST problem: The ParentheziedExpression's range does not include the parenthesis
Comment 3 Olivier Thomann CLA 2007-10-29 08:30:08 EDT 
I'll take a look.
Comment 4 Olivier Thomann CLA 2007-10-29 08:34:30 EDT 
Reproduced.
I am investigating.
Comment 5 Olivier Thomann CLA 2007-10-29 11:25:54 EDT 
Created attachment 81468 [details]
Proposed fix
Comment 6 Olivier Thomann CLA 2007-10-29 11:26:13 EDT 
Created attachment 81469 [details]
Regression test
Comment 7 Olivier Thomann CLA 2007-10-29 11:26:52 EDT 
Frederic,

Please review.
Comment 8 Frederic Fusier CLA 2007-10-31 12:51:17 EDT 
Created attachment 81740 [details]
Proposed patch

I would prefer to add new constructor on BinaryExpression and CombinedBinaryExpression to avoid missing the duplication of this important information (sourceStart, sourceEnd, bits) anywhere this pattern is encountered. 

This does not add extra time cost and, IMO, make the code safer. What do you think?
Comment 9 Olivier Thomann CLA 2007-11-05 20:04:02 EST 
+1.
Patch look good.
Comment 10 Frederic Fusier CLA 2007-11-06 06:12:08 EST 
Released for 3.4M4 in HEAD stream.
Comment 11 Eric Jodet CLA 2007-12-12 03:00:02 EST 
Verified for 3.4 M4 using build I20071211-0010
Comment 12 Roland Illig CLA 2008-08-11 03:16:50 EDT 
Will this bug be fixed in an update to Eclipse 3.3? Or do I have to upgrade to 3.4?