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