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

[Mike Duigou]

Thanks. That's an important clarification to include. Here's the revised text:

     * 
     * <p>Care must also be exercised when using collections that do not permit
     * calling the {@code contains} method with a {@code null} value. If either
     * collection does not permit {@code contains(null)} then both collections
     * must not contain {@code null} values.
     *

and the @throws text:

     * @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 contains(null)}.

Mike

On Dec 21 2010, at 13:15 , Eamonn McManus wrote:

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