Hi Claes,
On 01/08/18 23:41, Peter Levart wrote:
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?
I just noticed that AbstractSet does the same (catches CCE). That's
probably because of SortedSet subclasses too. So if you think
AbstractImmutableSet might be used for some immutable SortedSet
implementations in the future, you should probably leave it unchanged.
If elemen't equals() throws CCE it is violating the spec so it does not
matter what the outcome is (although probably propagating CCE might help
catching the bug). If you envision other AbstractImmutableSet subclasses
in the future, then perhaps it would be good to declare an abstract
hashCode() method in it. So implementations can't forget to define it.
When one sees that equals() is already defined, he might falsely assume
that hashCode() is defined too.
Regards, Peter
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
