Hi Claes,
On 01/08/18 21:57, Claes Redestad wrote:
Gosh! My intent was to carry over AbstractSet::equals verbatim, so I
have no idea how the size
check got lost in translation and passed through testing! Appears to
be another hole in the test
coverage, or I didn't run the right ones.
As to calculating o.size() as we go then I'm not sure: most
Set::size() impls in the JDK are O(1)
(including the ones here), and you lose a very fast fast path in
common cases for a hypothetical
improvement on some collection types whose size() isn't. Leaving it as
is is also sticking with
the status quo, which seems favorable for the scope of this RFE.
CCE is specified to be thrown by Set::contains/containsAll, so I think
we should play it safe and
leave it unchanged.
It's probably specified so to accommodate for SortedSet(s) which don't
use equals but compareTo / Comparator. But here you are calling
containsAll on an abstract Set with known implementation(s) which are
hash-based and CCE will never be thrown. If CCE is thrown from element's
equals() then it should probably propagate out of
AbstractImmutableSet.equals() and not silently translate to non-equality
of sets.
What do you think?
Regards, Peter
Thanks!
/Claes
On 2018-01-08 20:38, Peter Levart wrote:
Also, I don't think that ClassCastException should be caught here. It
should never be thrown by containsAll(c) anyway.
On 01/08/18 20:09, Peter Levart wrote:
Hi Claes,
Are you sure that AbstractImmutableSet.equals(Object) is correct?
@Override
public boolean equals(Object o) {
if (o == this)
return true;
if (!(o instanceof Set))
return false;
Collection<?> c = (Collection<?>) o;
try {
return containsAll(c);
} catch (ClassCastException | NullPointerException
unused) {
return false;
}
}
It seems to me that this implementation returns true when passing in
a sub-set of this. Should the method also check that the size(s) of
both sets are the same?
Regards, Peter
