Code Review Request for 4533691 (add Collections.EMPTY_SORTED_SET)

[Rémi Forax]

On 11/07/2011 01:28 AM, David Holmes wrote:
> 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.

The spec clearly says that headSet/tailSet creates subsets that should 
check one of their bounds
regardless if the master set is empty or not.


>
> 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? 

Yes.

> Is it simply inconsistent with other collections, but allowed by the 
> spec? 

No

> Could we in fact "fix" it? 

Yes

> If we can why didn't we?

perhaps because it's weird to have a view of a stateless collection
which is not stateless.

The other solution is to relax the spec to say that the returned set
may be not a restricted and make the IAE optional.

>
> Cheers,
> David
>
>> Mike

Rémi