On Sat, 1 Apr 2023 18:14:11 GMT, Michael Strauß <mstra...@openjdk.org> wrote:
>> `Observable{List/Set/Map}Wrapper.retainAll/removeAll` can be optimized for
>> some edge cases.
>>
>> 1. `removeAll(c)`:
>> This is a no-op if 'c' is empty.
>> For `ObservableListWrapper`, returning early skips an object allocation. For
>> `ObservableSetWrapper` and `ObservableMapWrapper`, returning early prevents
>> an enumeration of the entire collection.
>>
>> 2. `retainAll(c)`:
>> This is a no-op if the backing collection is empty, or equivalent to
>> `clear()` if `c` is empty.
>>
>> I've added some tests to verify the optimized behavior for each of the three
>> classes.
>
> Michael Strauß has updated the pull request incrementally with one additional
> commit since the last revision:
>
> addressed review comments, added tests
Looks good. Left a few minor comments.
modules/javafx.base/src/main/java/com/sun/javafx/collections/ObservableSequentialListWrapper.java
line 178:
> 176: @Override
> 177: public boolean addAll(int index, Collection<? extends E> c) {
> 178: if (index < 0 || index > size()) {
If you want, there are methods in `Objects` that do range checks, like
`Objects.checkIndex`.
Same for the other files.
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.
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.
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.
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.
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.
-------------
PR Review: https://git.openjdk.org/jfx/pull/751#pullrequestreview-1367987645
PR Review Comment: https://git.openjdk.org/jfx/pull/751#discussion_r1155208556
PR Review Comment: https://git.openjdk.org/jfx/pull/751#discussion_r1155217217
PR Review Comment: https://git.openjdk.org/jfx/pull/751#discussion_r1155217387
PR Review Comment: https://git.openjdk.org/jfx/pull/751#discussion_r1155219827
PR Review Comment: https://git.openjdk.org/jfx/pull/751#discussion_r1155220370
PR Review Comment: https://git.openjdk.org/jfx/pull/751#discussion_r1155219034
