On Thu, 30 Mar 2023 13:03:22 GMT, John Hendrikx <[email protected]> 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/ObservableListWrapper.java
> line 172:
>
>> 170: @Override
>> 171: public boolean removeAll(Collection<?> c) {
>> 172: if (backingList.isEmpty() || c.isEmpty()) {
>
> I think you should do an explicit `null` check here on `c` or swap the order
> of these arguments so it always throws an NPE here if `c` is `null` as per
> collection contract. If you don't, it will do this implicit `null` check
> just after `beginChange`, and as I don't see a `try/finally` there to call
> `endChange`, it would mean the wrapper / changeListBuilder gets in a bad
> state.
I've opted for implicit checks by always dereferencing `c`.
> modules/javafx.base/src/main/java/com/sun/javafx/collections/ObservableListWrapper.java
> line 195:
>
>> 193: @Override
>> 194: public boolean retainAll(Collection<?> c) {
>> 195: if (backingList.isEmpty()) {
>
> I think we need to check `c` for `null` here first to conform to the
> collection contract.
Done.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/751#discussion_r1155050099
PR Review Comment: https://git.openjdk.org/jfx/pull/751#discussion_r1155050071
