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

[Peter Levart]

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?

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
>