RFR: 8283063: Optimize Observable{List/Set/Map}Wrapper.retainAll/removeAll [v4]

[Michael Strauß]

On Sun, 2 Apr 2023 00:31:51 GMT, Nir Lisker <nlisker at openjdk.org> wrote:

>> Michael Strauß has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   addressed review comments, added tests
>
> modules/javafx.base/src/test/java/test/com/sun/javafx/collections/ObservableListWrapperTest.java line 41:
> 
>> 39: 
>> 40:     @Nested
>> 41:     class RemoveAllTest {
> 
> Empty line after class declaration.
> 
> Same for other places.

I see that all over the JavaFX codebase. Do we have any guidance for that?

> modules/javafx.base/src/test/java/test/com/sun/javafx/collections/ObservableListWrapperTest.java line 57:
> 
>> 55:             });
>> 56: 
>> 57:             list.removeAll(Collections.<String>emptyList());
> 
> You can use `List.of()` instead of `Collections.emptyList()` if you want here and in other places.

Done.

> modules/javafx.base/src/test/java/test/com/sun/javafx/collections/ObservableMapWrapperTest.java line 65:
> 
>> 63: 
>> 64:         @Test
>> 65:         public void testValueSetNullArgumentThrowsNPE() {
> 
> The values collection is not a `Set`, but the intention is clear. Same in other places.

Fixed that.

> modules/javafx.base/src/test/java/test/com/sun/javafx/collections/ObservableMapWrapperTest.java line 108:
> 
>> 106: 
>> 107:             var map2 = new ObservableMapWrapper<>(new HashMap<>(Map.of("k0", "v0", "k1", "v1", "k2", "v2")));
>> 108:             assertThrows(NullPointerException.class, () -> map2.entrySet().retainAll((Collection<?>) null));
> 
> Would advise to rename `map1` to `emptyMap` and `map2` to `notEmptyMap` or the like.
> 
> Same in other places.

Done.

> modules/javafx.base/src/test/java/test/com/sun/javafx/collections/ObservableSequentialListWrapperTest.java line 55:
> 
>> 53:         };
>> 54: 
>> 55:         assertDoesNotThrow(() -> list.addAll(Collections.emptyList()));
> 
> The `addAll` method here does not belong to `ObservableSequentialListWrapper`, but to `ModifiableObservableListBase`, which is tested in another file. Not sure if this method test helps.

Good catch, I've removed the method test.

-------------

PR Review Comment: https://git.openjdk.org/jfx/pull/751#discussion_r1155395211
PR Review Comment: https://git.openjdk.org/jfx/pull/751#discussion_r1155394964
PR Review Comment: https://git.openjdk.org/jfx/pull/751#discussion_r1155395046
PR Review Comment: https://git.openjdk.org/jfx/pull/751#discussion_r1155395091
PR Review Comment: https://git.openjdk.org/jfx/pull/751#discussion_r1155394994