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

[Peter Levart]

Hi Claes,

On 01/08/18 23:41, Peter Levart wrote:
> 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?

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

>
> 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
>>
>