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

Peter Levart peter.levart at gmail.com
Mon Jan 8 22:41:11 UTC 2018

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

More information about the core-libs-dev mailing list