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

[Mike Duigou]

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