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

[Claes Redestad]

Hi,

noticed I had replied off-list to Peter by accident. Sorry for the 
unintended delay..

I think we can settle for the simplified equals (as previously outlined) 
along with an abstract hashCode to remind ourselves it needs to be 
overridden:

http://cr.openjdk.java.net/~redestad/8193128/open.06/

As for the race that's can make equals() return true when the other set 
is cleared after calling size() on it but before iterating over the 
collection, then I think that's behavior you could provoke with most of 
the non-j.u.c Set implementations in the JDK (which typically defer to 
AbstractSet::equals) - including the existing Set.of() implementation - 
so I consider it out of scope for this improvement.

Thanks!

/Claes

On 2018-01-09 13:27, Peter Levart wrote:
> Hi Claes,
>
> On 01/09/2018 02:17 AM, Claes Redestad wrote:
>> 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. 
>
> That's nicer in my opinion. What to do with ClassCastException from 
> contains(e)? Perhaps just document that any future implementation that 
> might throw it from contains() should also modify the equals() method 
> and catch it there.
>
> And if you already do explicit iteration over elements, you could as 
> well count them and do the size check with counted size at the end. 
> Imagine the following scenario:
>
> ConcurrentHashMap m = new ChocurrentHashMap();
> m.put(1, 1);
> m.put(2, 2);
> m.put(3, 3);
>
> Set s = Set.of("a", "b", "c");
>
> Thread 1:
>
> s.equals(m.keySet())
>
> Thread 2:
>
> m.clear();
>
> It is surprising when Thread 1 encounters 'true' as the result of the 
> equals() above which can happen when code checks for sizes separately. 
> Iterating CHM.keySet() does not provide a snapshot of the content 
> either, but counting iterated and compared elements nevertheless 
> exhibits less surprising behavior. I don't know if it matters though.
>
> Regards, Peter
>