Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.
Bug 325000 - Project properties not sorted on IBM VMs
Summary: Project properties not sorted on IBM VMs
Status: VERIFIED FIXED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: Resources (show other bugs)
Version: 3.7   Edit
Hardware: PC Windows 7
: P3 normal (vote)
Target Milestone: 3.7 M5   Edit
Assignee: Malgorzata Janczarska CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-09-10 14:19 EDT by Markus Keller CLA
Modified: 2011-01-26 04:20 EST (History)
2 users (show)

See Also:
Szymon.Brandys: review+


Attachments
possible fix (1.80 KB, patch)
2010-09-10 14:19 EDT, Markus Keller CLA
no flags Details | Diff
Markus patch tuned up. (5.30 KB, patch)
2010-12-01 13:17 EST, Malgorzata Janczarska CLA
no flags Details | Diff
Removed String.valueOf (5.32 KB, patch)
2010-12-23 04:42 EST, Malgorzata Janczarska CLA
no flags Details | Diff
Can be applied for generified class. (5.77 KB, patch)
2011-01-04 11:54 EST, Malgorzata Janczarska CLA
no flags Details | Diff
Can be applied for generified class - corrected (5.77 KB, patch)
2011-01-05 05:44 EST, Malgorzata Janczarska CLA
Szymon.Brandys: iplog+
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Markus Keller CLA 2010-09-10 14:19:32 EDT
Created attachment 178643 [details]
possible fix

I20100909-1700

Project properties are not sorted alphabetically on IBM VMs. The problem is that ProjectPreferences#save() uses SortedProperties, which assumes that Properties#store(..) calls keys() to enumerate the properties.

But this is not specified, and IBM JREs (e.g. 60sr8-20100409_01) iterate over entrySet(). If you want to stay on the edge, you can release the attached patch (overrides entrySet() in a similar way, but breaks its contract since the returned set is not a live view). Otherwise, you have to implement the storing yourself.
Comment 1 Markus Keller CLA 2010-10-22 05:55:40 EDT
Could this please be released for 3.7?

The attached patch is currently safe and fixes the bug. SortedProperties is only used locally and the only methods called on it that are not under our control are #put(..) and #store(..). I don't expect #put(..) to use #entrySet() and rely on the write-through feature. #store(..) should never change the contents so it also doesn't care about whether the #entrySet() is live.
Comment 2 Malgorzata Janczarska CLA 2010-12-01 13:17:36 EST
Created attachment 184282 [details]
Markus patch tuned up.

I have made some corrections to Markus patch:
1. Added a test
2. Rewritten it to java 1.5 since resources are now compatible with it
3. User String.valueOf instead of just casting the object to string. It's safer, but it shouldn't meter because I don't think that we have to do with any object different than a String
4. I made an equivalent comparator for the keys() method to prevent differences in order using those two methods.
Comment 3 Szymon Brandys CLA 2010-12-03 06:52:55 EST
The patch looks good. I will release it right after M4.

> 4. I made an equivalent comparator for the keys() method to prevent differences in order using those two methods.

TreeSet contains 'any objects which are comparable to each other either using their natural order or a specified Comparator'. Since, as you wrote, we expect only Strings there, the natural order seems to be ok.
Comment 4 Malgorzata Janczarska CLA 2010-12-03 07:48:13 EST
> TreeSet contains 'any objects which are comparable to each other either using
> their natural order or a specified Comparator'. Since, as you wrote, we expect
> only Strings there, the natural order seems to be ok.
In this particular case yes, this only prevents from problems when somebody break the contact and ads a preference that is not a string.
Comment 5 Markus Keller CLA 2010-12-03 08:23:57 EST
I agree with Szymon. Only EclipsePreferences#convertToProperties(..) puts elements into the Properties, and if that is ever changed, I'd rather see an exception than a silent String#valueOf(..) that hides the problem.

You could also make the proper usage explicit by changing result.put(..) to result.setProperty(..).
Comment 6 Malgorzata Janczarska CLA 2010-12-23 04:42:31 EST
Created attachment 185753 [details]
Removed String.valueOf

OK, you've convinced me. I removed the String.valueOf and used casting instead. How do you like the solution now?
Comment 7 Markus Keller CLA 2011-01-04 09:05:22 EST
(In reply to comment #6)
The comparator in the keys() method is unnecessary and should therefore not be added (default comparator does the same).

I also find it a bit strange that you only generified some of the code but left other warnings in the file. Refactor > Infer Generic Type Arguments resolves the other problems for free.
Comment 8 Szymon Brandys CLA 2011-01-04 10:06:13 EST
(In reply to comment #7)
> I also find it a bit strange that you only generified some of the code but left
> other warnings in the file. Refactor > Infer Generic Type Arguments resolves the
> other problems for free.

You are right. I fixed the generic warnings first so the patch would contain bug related changes only.
Comment 9 Malgorzata Janczarska CLA 2011-01-04 11:54:16 EST
Created attachment 186028 [details]
Can be applied for generified class.

I corrected the patch, so you can now apply it on generified class.
Comment 10 Malgorzata Janczarska CLA 2011-01-05 05:44:03 EST
Created attachment 186080 [details]
Can be applied for generified class - corrected

The previous patch contained an error in test. I'm attaching corrected version.
Comment 11 Szymon Brandys CLA 2011-01-05 06:20:17 EST
In HEAD. Thanks Gosia.
Comment 12 Malgorzata Janczarska CLA 2011-01-26 04:20:32 EST
Verified by code inspection.