On Tue, 14 Sep 2021 21:09:34 GMT, Roger Riggs <rri...@openjdk.org> wrote:
>> Hmm, so if someone has subclassed `Properties` and overridden `entrySet` for >> the purpose of ordering entries, that trick will no longer work with the new >> code. I wonder - should we sort entries only when the >> `java.util.Properties.storeDate` is also defined and not empty? Or should we >> simply state in the release notes that such tricks will no longer work? > > I think we want the entries to be sorted by default; that is the most useful > to the most people. > Checking for the overloaded method seems like trying too hard. > Changing the entrySet to return them sorted (always) would add overhead in a > lot of cases but would allow a subclass to re-sort them as they see fit. > A half measure would be to sort only if the properties instance was not > subclassed and spec it that way as an implementation detail. Good catch regarding the possibility of entrySet() being overridden. > I think we want the entries to be sorted by default; that is the most useful > to the most people. Agreed > Changing the entrySet to return them sorted (always) would add overhead in a > lot of cases but would allow a subclass to re-sort them as they see fit. Agreed - I think changing the implementation of entrySet to return an ordered set is an unnecessary overhead in the context of what's being targeted in this PR. > A half measure would be to sort only if the properties instance was not > subclassed and spec it that way as an implementation detail. I think just checking the properties instance type to see if it is subclassed is perhaps too big a penalty for those applications that do subclass java.util.Properties but don't override the entrySet() method. Such applications/subclasses will then never be able to use this new implementation where we write out the properties in the natural order. Perhaps a middle ground would be your other option: > Checking for the overloaded method seems like trying too hard. IMO, I don't think we would be trying too hard to identify this. I have updated this PR to try and articulate this both in terms of the implementation as well as the updated javadoc of this method. Tests have then been introduced to verify this case of Properties subclasses. This https://github.com/openjdk/jdk/pull/5372/commits/79d1052775dd8e98fb7078710eda0fd6dd83b164 is the relevant commit in the updated PR. I understand that we haven't come to an agreement on this aspect yet, but it was easier to show it in terms of code and spec, so I decided to include this commit in the PR. I will of course revert it or change it appropriately depending on how we want to deal with this case. ------------- PR: https://git.openjdk.java.net/jdk/pull/5372
