On Wed, 15 Sep 2021 06:11:31 GMT, Jaikiran Pai <j...@openjdk.org> wrote:
>> 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. > > 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 79d1052 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. Hmm. Interesting idea - it looks like it could work given that properties as a private `EntrySet` class used for its entry set implementation... @RogerRiggs what do you think? ------------- PR: https://git.openjdk.java.net/jdk/pull/5372
