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

[Chris Hegarty]

Mike,

On 12/21/10 09:38 PM, Mike Duigou wrote:
> 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)}.

My concern with this revised wording is that you are now specifying that 
the implementation must use contains() ( and not be implemented using a 
different strategy ). I guess an alternative implementation is unlikely, 
but this does appear overly restricting.

I wonder if its really necessary to add text to the NPE. A cautionary 
note may be sufficient. We could also throw ClassCastException, but 
there is no mention of it in the spec.

Sorry for being a pain about this, I'm just concerned with adding overly 
restricting spec.

Have we thought about catching/swallowing these exceptions?

-Chris.


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