[11] RFR: 8193128: Reduce number of implementation classes returned by List/Set/Map.of() (original) (raw)

Claes Redestad claes.redestad at oracle.com
Tue Jan 9 01:17:52 UTC 2018

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

More information about the core-libs-dev mailing list