On Sat, 10 Dec 2022 21:55:03 GMT, Nir Lisker <[email protected]> wrote:
>> John Hendrikx has updated the pull request incrementally with three
>> additional commits since the last revision:
>>
>> - Adjusted ReadOnlyListProperty#equals to be a bit more modern Java
>> - Remove SuppressWarnings in ReadOnlyListProperty
>> - Use assignment in instanceof
>
> modules/javafx.base/src/main/java/javafx/beans/property/ReadOnlySetProperty.java
> line 117:
>
>> 115: @SuppressWarnings("unchecked")
>> 116: Set<E> c = (Set<E>) obj; // safe cast as elements are only
>> referenced
>> 117:
>
> I suggest to change it similarly to list:
>
>
> if (obj == this) {
> return true;
> }
>
> if (!(obj instanceof Set<?> otherSet) || otherSet.size() != size()) {
> return false;
> }
> try {
> return containsAll(otherSet);
> } catch (ClassCastException | NullPointerException unused) {
> return false;
> }
>
>
> I find it odd that there is a need to catch these exceptions, but
> `AbstractSet` does so too.
>
> Same for the `Map` variant.
NPE is probably because some underlying sets will throw NPE even for operations
like `contains(null)` (the immutable variants from `List.of` and `Set.of` are
bad offenders in this area). CCE maybe for sets that use comparators
internally.
I've adjusted it as you suggested, and same for map.
-------------
PR: https://git.openjdk.org/jfx/pull/969
