Review for CR 6728865 : Improved heuristics for Collections.disjoint() [updated]

[Eamonn McManus]

I think that is still not quite right. The spec for Collection says that 
a Collection that does not support adding null values may or may not 
support looking them up. So in both places where you say "does not 
permit null values" I think you should probably say "does not permit 
looking up null values".

Éamonn

On 21/12/2010 20:35, Mike Duigou wrote:
> On Dec 21 2010, at 02:43 , Chris Hegarty wrote:
>
>> On 12/21/10 02:24 AM, David Holmes wrote:
>>> Functionality looks okay to me.
>>>
>>> I think those spec/doc clarifications may need to go through CCC though.
>> I agree with David, a CCC request should be filed for the spec changes. We should agree the spec changes on this mailing list before proposing them.
>>
>> I understand where you're coming from with this spec change, but I think the additional text may be too restricting.
>>
>>   "@throws NullPointerException ......... or if one collection
>>    contains {@code null} and the other collection does not permit
>>    {@code null} values."
>>
>> For example, the following would be required to throw NPE ( but I don't believe your impl does):
>>
>>   Set set = new HashSet();
>>   set.add(null);
>>   PriorityQueue pq = new PriorityQueue();
>>   Collections.disjoint(set, pq);
>>
>> I think we may have to be a little more relaxed here, maybe just a cautionary note, "it may happen"???
> You are correct that it's not guaranteed that NPE will be thrown. Here's the amended text for the main javadoc:
>
>       *<p>Care must also be exercised when using a mix of collections that
>       * permit {@code null} values and those that do not. If either
>       * collection does not permit {@code null} values then {@code null} must
>       * not be a value in either collection.
>       *
>
> and this is the revised @throw NullPointerException:
>
>       * @throws NullPointerException if either collection is {@code null}. May
>       * also be thrown if one collection contains a {@code null} value and the
>       * other collection does not permit {@code null} values.
>
>
> Note that the descriptive paragraph says "must not" because we don't commit to which collection is used for contains() and the @throw says "may" because, per your example, if the collection not permitting null is used for iteration then NPE will not be thrown.
>
> Mike
>
>> -Chris.
>>
>>> David
>>>
>>> Mike Duigou said the following on 12/21/10 11:48:
>>>> I've updated the webrev with Ulf's feedback.
>>>> http://cr.openjdk.java.net/~mduigou/6728865.2/webrev/
>>>>
>>>> The old heuristics:
>>>>
>>>> 1. If c1 is a Set then iterate over c2.
>>>>
>>>> 2. Iterate over the smaller Collection.
>>>>
>>>>
>>>> I believe that the || in the original should have been a&&
>>>>
>>>> I've rearranged it as branches in my revision.
>>>>
>>>>
>>>> The new heuristics:
>>>>
>>>> 1. If c1 is a Set then iterate over c2.
>>>>
>>>> 2. If c2 is a Set then iterate over c1.
>>>>
>>>> 3. If either collection is empty then result is always true.
>>>>
>>>> 4. Iterate over the smaller Collection.
>>>>
>>>> Mike
>>>>
>>>>
>>>>
>>>> On Dec 19 2010, at 16:42 , David Holmes wrote:
>>>>
>>>>> Hi Mike,
>>>>>
>>>>> Mike Duigou said the following on 12/20/10 10:29:
>>>>>> I have updated the webrev for CR 6728865 with Rémi's feedback. The
>>>>>> new webrev is at:
>>>>>> http://cr.openjdk.java.net/~mduigou/6728865.1/webrev/
>>>>>> The size() comparisons are now done only when both c1 and c2 are not
>>>>>> sets and I have removed the isEmpty() micro-optimization.
>>>>> So to summarise this change:
>>>>>
>>>>> 1. The original code checked for c1 being a set and not c2, but not
>>>>> vice-versa - this fixes that
>>>>>
>>>>> 2. This code adds an optimization when they are both not sets but at
>>>>> least one is empty
>>>>>
>>>>> Did I miss anything?
>>>>>
>>>>> Seems functionally sound to me, but I can't attest to any performance
>>>>> benefits.
>>>>>
>>>>> Cheers,
>>>>> David Holmes