Bugzilla – Full Text Bug Listing
| Summary: | Project properties not sorted on IBM VMs | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | Markus Keller <markus.kell.r> | ||||||||||||
| Component: | Resources | Assignee: | Malgorzata Janczarska <malgorzata.tomczyk> | ||||||||||||
| Status: | VERIFIED FIXED | QA Contact: | |||||||||||||
| Severity: | normal | ||||||||||||||
| Priority: | P3 | CC: | malgorzata.tomczyk, Szymon.Brandys | ||||||||||||
| Version: | 3.7 | Flags: | Szymon.Brandys:
review+
|
||||||||||||
| Target Milestone: | 3.7 M5 | ||||||||||||||
| Hardware: | PC | ||||||||||||||
| OS: | Windows 7 | ||||||||||||||
| Whiteboard: | |||||||||||||||
| Attachments: |
|
||||||||||||||
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. 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.
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.
> 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.
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(..). 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?
(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. (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. Created attachment 186028 [details]
Can be applied for generified class.
I corrected the patch, so you can now apply it on generified class.
Created attachment 186080 [details]
Can be applied for generified class - corrected
The previous patch contained an error in test. I'm attaching corrected version.
In HEAD. Thanks Gosia. Verified by code inspection. |
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.