Code Review Request for 4533691 (add Collections.EMPTY_SORTED_SET)

[David Holmes]

Hi Mike,

On 5/11/2011 2:29 AM, Mike Duigou wrote:
>
> On Nov 3 2011, at 17:40 , David Holmes wrote:
>> I see that you have pushed a version of this change and that I was listed as a reviewer. However I never saw an updated webrev and there was no response to my query below. In fact I never saw any response to any of the reviewers comments on this.
>
> I missed your question about the range on an empty set. My comments below.
>
> Removing EMPTY_SORTED_SET was the only other comment to my knowledge. Darryl adapted the patch and EMPTY_SORTED_SET was not part of the commit. Since the change was removal of a newly proposed field an additional review cycle wasn't thought to be necessary. Perhaps incorrectly?

I certainly expected to see a response to the comments and to see 
definitive reviewer responses to the effect "I agree with this change" 
before it was pushed. Even if the only comment had been to drop 
EMPTY_SORTED_SET, I would have expected an email stating that this had 
been done and the amended changeset pushed. But in this case there was 
more to address.

More inline ...


>> On 31/10/2011 10:52 AM, David Holmes wrote:
>>> On 29/10/2011 8:44 AM, Darryl Mocek wrote:
>>>> Additional Notes to Reviewers:
>>>> The sets resulting from tailSet() headSet() and subSet() normally
>>>> include the range which was used to create them. Using these methods
>>>> with emptySortedSet() does not currently set a range on the resulting
>>>> sets.
>>>
>>> What is the range on an empty set?
>
> The results of Collections.emptySortedSet() will behave differently than Collections.unmodifiableSortedSet(new TreeSet()) for cases involving headSet, tailSet and subSet. These three operations remember the range used to create the sub set in addition to providing the elements within that range. The range is considered in a few cases such add item addition (added items must be within the range of the subset) and any subset created from a range sub set must lie within the range. Relevant to emptySortedSet is the creation of sub sets off of a ranged sub set.
>
> public static void main(String[] args) {
>      SortedSet<Double>  foo = Collections.unmodifiableSortedSet(new TreeSet<Double>());
>
>      SortedSet<Double>  two = foo.headSet(Double.valueOf(2.0));  // range<  2.0
>      SortedSet<Double>  one = two.headSet(Double.valueOf(1.0));
>      try {
>          SortedSet<Double>  three = two.headSet(Double.valueOf(3.0)); // IAE "toKey out of range"
>      } catch (IllegalArgumentException expected) {
>          expected.printStackTrace(System.err);
>      }
>
>      two = foo.tailSet(Double.valueOf(2.0)); // range>= 2.0
>      try {
>          one = two.tailSet(Double.valueOf(1.0));  // IAE "fromKey out of range"
>      } catch (IllegalArgumentException expected) {
>          expected.printStackTrace(System.err);
>      }
>      SortedSet<Double>  three = two.tailSet(Double.valueOf(3.0));
>
>      two = foo.subSet(Double.valueOf(2.0),Double.valueOf(3.0)); // 2.0<= range<  3.0
>      try {
>          one = two.subSet(Double.valueOf(1.0),Double.valueOf(2.0));  // IAE "fromKey out of range"
>      } catch (IllegalArgumentException expected) {
>          expected.printStackTrace(System.err);
>      }
>      three = two.subSet(Double.valueOf(2.0),Double.valueOf(3.0));
> }
>
> This program throws three IAE exceptions. Replacing the first statement with:
>
> SortedSet<Double>  foo = Collections.emptySortedSet();
>
> would result in no IAE exceptions because they emptySortedSet() does not track ranges for sub sets derived with headSet(), tailSet() and subSet(). The range on emptySortedSet and derived sets is always fully open.

Naively perhaps I would have expected the range on an empty set to be 
fully closed, rather than fully open.

That aside it is unclear what this means from an API specification 
perspective. Is this difference in behaviour an error ie does it violate 
the spec? Is it simply inconsistent with other collections, but allowed 
by the spec? Could we in fact "fix" it? If we can why didn't we?

Cheers,
David

> Mike