Bug 188103

Summary:

[1.5][compiler] Inappropriate usage of HashSet

Product:

[Eclipse Project] JDT

Reporter:

Andrey Pavlenko <andrey.a.pavlenko>

Component:

Core

Assignee:

Philipe Mulet <philippe_mulet>

Status:

VERIFIED FIXED

QA Contact:

Severity:

normal

Priority:

Flags:

Olivier_Thomann: review+
jerome_lanneluc: review+

Version:

3.2

Target Milestone:

3.3 RC2

Hardware:

OS:

Windows XP

Whiteboard:

Attachments:

Description	Flags
Proposed patch	none
Patch including test change	none
Better patch	none
Even better patch	none
Even better patch 2	none

Description Andrey Pavlenko

2007-05-21 11:12:19 EDT

Build ID: 3.2

Steps To Reproduce:
1. build the latest Harmony jre
2. dowload EUT runniing script from jira-2038
3. update eut.properties: set test.jre.home= to Harmony jre home
4. run "ant -Dtests=jdtcorecompiler" 


More information:
The method org.eclipse.jdt.internal.compiler.lookup.Scope.minimalErasedCandidates(TypeBinding[], Map) uses HashSet for parameter types storing, but, according to the spec, HashSet does not keep the insertion order. This is the cause of 58 failures of jdtcorecompiler tests on Harmony VM. See the following link for more details: http://issues.apache.org/jira/browse/HARMONY-3393
If I replace HashSet with LinkedHashSet all of the tests successfully pass.

Comment 1 Philipe Mulet

2007-05-21 11:26:02 EDT

It is using a HashMap.

Comment 2 Philipe Mulet

2007-05-21 11:35:43 EDT

Ignore previous comment. Specific invocations are indeed stored into a HashSet, which I presume is what you were referring too.

Comment 3 Philipe Mulet

2007-05-21 11:43:03 EDT

Problem is that LinkedHashSet was only introduced in 1.4, and this part of compiler cannot use 1.4 specific additions.

Comment 4 Andrey Pavlenko

2007-05-21 12:01:16 EDT

Yes, you are right. So, maybe it's reasonable to use LinkedList instead of HashSet?

Comment 5 Philipe Mulet

2007-05-21 12:33:11 EDT

Created attachment 67990 [details]
Proposed patch

Proposed alternate solution using a list instead.

Comment 6 Philipe Mulet

2007-05-21 15:28:20 EDT

Adjusted GenericTypeTest#test0985.

Comment 7 Philipe Mulet

2007-05-21 15:31:32 EDT

Created attachment 68019 [details]
Patch including test change

Comment 8 Philipe Mulet

2007-05-21 18:15:55 EDT

Created attachment 68031 [details]
Better patch

Avoid allocating list of size 1

Comment 9 Philipe Mulet

2007-05-21 18:16:33 EDT

Olivier - pls review

Comment 10 Philipe Mulet

2007-05-21 18:17:27 EDT

Andrey - are you able to apply the patch, and assess that it effectively addresses the Harmony issue ?

Comment 11 Philipe Mulet

2007-05-21 18:18:33 EDT

Kent - pls review

Comment 12 Philipe Mulet

2007-05-22 03:45:40 EDT

Created attachment 68073 [details]
Even better patch

Removed unnecessary check for size==1, since lists are only created when size > 1.

Comment 13 Andrey Pavlenko

2007-05-22 05:17:36 EDT

Phillipe, the patch is seems to be ok, now the tests pass on Harmony. Thanks.

Comment 14 Philipe Mulet

2007-05-22 08:20:08 EDT

Created attachment 68103 [details]
Even better patch 2

Using a TypeBinding[] instead of a List.

Comment 15 Olivier Thomann

2007-05-22 10:05:09 EDT

+1

Comment 16 Philipe Mulet

2007-05-22 10:36:21 EDT

No sign of Kent - Jerome pls review.

Comment 17 Philipe Mulet

2007-05-22 11:41:58 EDT

Released for 3.3RC2.
Fixed

Comment 18 Jerome Lanneluc

2007-05-24 11:33:02 EDT

Verified for 3.3RC2 using I20070524-0010