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

[Claes Redestad]

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