On Sat, 10 Dec 2022 21:55:03 GMT, Nir Lisker <nlis...@openjdk.org> 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

Reply via email to