Hi,
On 2018-01-09 00:04, Peter Levart wrote:
Hi Claes,
On 01/08/18 23:41, Peter Levart wrote:
Hi Claes,
On 01/08/18 21:57, Claes Redestad wrote:
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
you're right that for this closed world of AbstractImmutableSets we can
deduce that CCE can't be thrown and
ignore that possibility for now. Not sure it matters much.
Adding a public abstract int hashCode(); might be a kindness. I'll add
that unless there are objections. :-)
If we took it a step further we could save us the trouble of throwing
and swallowing NPEs in
AbstractImmutableSet::equals when the other set contains null, which
might be an optimization
in that case:
@Override
public boolean equals(Object o) {
if (o == this)
return true;
if (!(o instanceof Set))
return false;
Collection<?> c = (Collection<?>) o;
if (c.size() != size())
return false;
for (Object e : c)
if (e == null || !contains(e))
return false;
return true;
}
It might be rare enough that it's not worth bothering with, though.
/Claes
