On Sat, 1 Apr 2023 07:32:20 GMT, John Hendrikx <jhendr...@openjdk.org> wrote:
>> Michael Strauß has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains five commits:
>>
>> - Merge branch 'master' into fixes/JDK-8283063
>>
>> # Conflicts:
>> #
>> modules/javafx.base/src/main/java/com/sun/javafx/collections/ObservableListWrapper.java
>> #
>> modules/javafx.base/src/main/java/com/sun/javafx/collections/ObservableMapWrapper.java
>> #
>> modules/javafx.base/src/main/java/com/sun/javafx/collections/ObservableSequentialListWrapper.java
>> #
>> modules/javafx.base/src/main/java/com/sun/javafx/collections/ObservableSetWrapper.java
>> - address review comments
>> - refactored removeAll/retainAll optimizations
>> - Optimize removeAll/retainAll for Observable{List/Set/Map}Wrapper
>> - Failing test
>
> modules/javafx.base/src/main/java/com/sun/javafx/collections/ObservableMapWrapper.java
> line 326:
>
>> 324: if (backingMap.isEmpty()) {
>> 325: return false;
>> 326: }
>
> Passing `null` is always an error, and I think here you still need to throw
> an NPE first when `c` is `null`, even if the backing map is empty. From
> `AbstractColection` for example:
>
> public boolean retainAll(Collection<?> c) {
> Objects.requireNonNull(c);
>
> ...
>
> I realize this was already incorrect, so perhaps out of scope for your PR.
I've fixed problems like this and added comments in all places where a null
check is important.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/751#discussion_r1155153443
