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