Bug 213519 - [extract method] Missing return value, while extracting code out of a loop
Summary: [extract method] Missing return value, while extracting code out of a loop
Status: VERIFIED FIXED
Alias: None
Product: JDT
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 3.4   Edit
Hardware: PC Windows XP
: P3 normal (vote)
Target Milestone: 3.6 M1   Edit
Assignee: Benjamin Muskalla CLA
QA Contact:
URL:
Whiteboard:
Keywords:
: 334558 335674 (view as bug list)
Depends on:
Blocks:
 
Reported: 2007-12-19 14:53 EST by Michael Klenk CLA
Modified: 2011-01-28 08:15 EST (History)
8 users (show)

See Also:
markus.kell.r: review+


Attachments
patch + testcases (9.09 KB, patch)
2009-07-02 20:21 EDT, Benjamin Muskalla CLA
markus.kell.r: iplog+
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Klenk CLA 2007-12-19 14:53:37 EST
Build ID: M20071023-1652

Steps To Reproduce:
There is a curious behavior of extract method when extracting a code snippet out of a loop after a for(Class var : Colection) construction.


More information:
Sample Code:
------------------------------------
Line of code to extract: list.add(a++);
------------------------------------

Code where Extract Method returns an int as return value of the extracted snippet:
Good Code:
------------------------------------
ArrayList<Integer> list = new ArrayList<Integer>();
int a = 0;
for (int c = 0; c < 10; c++) {
   list.add(a++);
}
System.out.println(list.toString());
------------------------------------
Method signature:
private static int someMethodName(ArrayList<Integer> list,int a)

After adding a for loop in front of the counting loop, the Extract Method return value is void.

Bad Code:
------------------------------------
ArrayList<Integer> list = new ArrayList<Integer>();
for (Integer var: list) {}
int a = 0;
for (int c = 0; c < 10; c++) {
   list.add(a++);
}
System.out.println(list.toString());
------------------------------------
Method signature:
private static void someMethodName(ArrayList<Integer> list,int a)
Comment 1 Olivier Thomann CLA 2007-12-19 14:59:23 EST
Move to JDT/UI
Comment 2 Dani Megert CLA 2008-01-07 12:27:36 EST
Markus please investigate.
Comment 3 Benjamin Muskalla CLA 2009-07-02 20:21:58 EDT
Created attachment 140727 [details]
patch + testcases

Attached is a patch with several testcases.
The problem was that the LoopReentranceVisitor is only set once with the first occurence of a loop and discarded later if the node under investigation is not part of the loop. I moved the check before the creation of the visitor which matches the right loop.
One thing which is still problematic: nested loops. But I think this is worth a FUP of this bug
Comment 4 Markus Keller CLA 2009-07-13 12:48:46 EDT
Thanks, released to HEAD (I added the missing return_out/A_test729.java).

> One thing which is still problematic: nested loops. But I think this is worth a
> FUP of this bug.

Yep, that's a separate (albeit related) problem. Could you please open a new bug, reference this bug's comment 3 in the description (to speed up the analysis when fixing that bug), and add a failing example if possible?
Comment 5 Dani Megert CLA 2009-08-04 06:12:07 EDT
The 'Bad Code' example from comment 0 still doesn't work for me using I20090803-1800 when I just select "list.add(a++)": I get a dialog with a cryptic text. Markus said he'll file a bug for that. Please post it here.

As stated in comment 3 and 4 nested loops are still broken and I could not see a new bug yet. Raksha, that's probably the issue you saw. Please file the bug report and post it here.
Comment 6 Markus Keller CLA 2009-08-04 06:20:35 EDT
> The 'Bad Code' example from comment 0 still doesn't work for me using
> I20090803-1800 when I just select "list.add(a++)"

That's bug 285551.
Comment 7 Raksha Vasisht CLA 2009-08-04 07:01:37 EDT
(In reply to comment #5)

> As stated in comment 3 and 4 nested loops are still broken and I could not see
> a new bug yet. Raksha, that's probably the issue you saw. Please file the bug
> report and post it here.
> 

Filed bug 285554 to track this.
Comment 8 Dani Megert CLA 2009-08-04 07:02:44 EDT
Marking as VERIFIED, given we have the FUPS.
Comment 9 Deepak Azad CLA 2011-01-17 22:56:51 EST
*** Bug 334558 has been marked as a duplicate of this bug. ***
Comment 10 Deepak Azad CLA 2011-01-28 08:15:01 EST
*** Bug 335674 has been marked as a duplicate of this bug. ***