Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.
Bug 366605 - [Compatibility] Failures in LeakTestSuite
Summary: [Compatibility] Failures in LeakTestSuite
Status: RESOLVED FIXED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 4.2   Edit
Hardware: All All
: P3 normal (vote)
Target Milestone: 4.2 M5   Edit
Assignee: Remy Suen CLA
QA Contact: Remy Suen CLA
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 366451 381873
  Show dependency tree
 
Reported: 2011-12-13 14:10 EST by Remy Suen CLA
Modified: 2012-06-06 11:11 EDT (History)
1 user (show)

See Also:

Attachments
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 Remy Suen CLA 2011-12-13 14:10:08 EST 
We have three failures here.

-testSimpleViewLeak
-testBug2555005SiteLeak
-testBug265449PropertiesLeak
Comment 1 Remy Suen CLA 2011-12-13 15:16:43 EST 
(In reply to comment #0)
> -testSimpleViewLeak
> -testBug265449PropertiesLeak

Fixed.

http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=2f36b46905cbc7fefebe121ad272532e226064e5
Comment 2 Remy Suen CLA 2011-12-13 15:36:00 EST 
(In reply to comment #1)
> (In reply to comment #0)
> > -testSimpleViewLeak
> > -testBug265449PropertiesLeak

Correction. testBug2555005SiteLeak was fixed.

testBug265449PropertiesLeak is now fixed also.
http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=fbf0e39912eed245b339765f927f4c00de16170d
Comment 3 Thomas Schindl CLA 2011-12-13 18:07:31 EST 
(In reply to comment #1)
> (In reply to comment #0)
> > -testSimpleViewLeak
> > -testBug265449PropertiesLeak
> 
> Fixed.
> 
> http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=2f36b46905cbc7fefebe121ad272532e226064e5

Remy I'm not sure but the fix looks strange because I think you could leave out items because of the remove not. 

Take this structure:
MToolBar
MOpaqueToolItem
MOpaqueToolItem

If i read the code appropriately you would not remove the second MOpaqueToolItem or am I mistaken

Wouldn't it be better to use an Iterator and call iterator.remove()?
Comment 4 Thomas Schindl CLA 2011-12-13 18:17:27 EST 
(In reply to comment #2)
> (In reply to comment #1)
> > (In reply to comment #0)
> > > -testSimpleViewLeak
> > > -testBug265449PropertiesLeak
> 
> Correction. testBug2555005SiteLeak was fixed.
> 
> testBug265449PropertiesLeak is now fixed also.
> http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=fbf0e39912eed245b339765f927f4c00de16170d

Same comment as the other one. I'm reopening this please close if you don't agree that this could is bogus and an iterator is the better and correct solution.
Comment 5 Remy Suen CLA 2011-12-13 18:34:24 EST 
(In reply to comment #3)
> Remy I'm not sure but the fix looks strange because I think you could leave out
> items because of the remove not. 
> 
> Take this structure:
> MToolBar
> MOpaqueToolItem
> MOpaqueToolItem
> 
> If i read the code appropriately you would not remove the second
> MOpaqueToolItem or am I mistaken
> 
> Wouldn't it be better to use an Iterator and call iterator.remove()?

I'm calling remove on the iterating 'i' local variable and also decrementing it in the next line. It seems right to me but I could be wrong.
Comment 6 Thomas Schindl CLA 2011-12-13 18:39:24 EST 
Ah missed the decrement but I think using an iterator is better not?
Comment 7 Remy Suen CLA 2011-12-13 18:41:06 EST 
(In reply to comment #6)
> Ah missed the decrement but I think using an iterator is better not?

The code is certainly easier to read that way. I will rewrite the code.
Comment 8 Remy Suen CLA 2012-01-19 10:59:31 EST 
(In reply to comment #6)
> Ah missed the decrement but I think using an iterator is better not?

Done.
http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=aad73d8ba1f576f62764828a8d53f37a467e191d