On Sat, 1 Apr 2023 08:00:33 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/javafx/collections/ModifiableObservableListBase.java
> line 142:
>
>> 140:
>> 141: return;
>> 142: }
>
> I'm really happy you also added the index range checks, not only does it
> conform to the contract better, it will help expose bugs in caller code (or
> even the JavaFX code as Observable* classes are used in many places, and who
> knows what bugs might lurk in some of the more complicated classes).
>
> I'm not quite sure how this check is suppose to work though; does this need a
> comment ?
> Suggestion:
>
> if (fromIndex == toIndex) { // distinguish between the clear and
> remove(int) call
> if (fromIndex < 0 || fromIndex > size()) {
> throw new IndexOutOfBoundsException("Index: " + fromIndex);
> }
>
> return;
> }
>
>
> I think that would be a bit too cryptic for me...
The index check is only really necessary for the optimized code path, since
calling `listIterator` later in the method would catch an invalid index.
However, I've opted to always check the index at the beginning of the method.
> modules/javafx.base/src/test/java/test/com/sun/javafx/collections/ObservableSequentialListWrapperTest.java
> line 60:
>
>> 58:
>> 59: @Test
>> 60: public void
>> testAddAllWithEmptyCollectionArgumentAndInvalidIndexThrowsIOOBE() {
>
> How about a variant that doesn't pass an empty collection, `-1` should still
> fail, and for example `100` should also fail.
Done.
> modules/javafx.base/src/test/java/test/javafx/collections/ModifiableObservableListBaseTest.java
> line 61:
>
>> 59:
>> 60: @Test
>> 61: public void
>> testAddAllWithEmptyCollectionArgumentAndInvalidIndexThrowsIOOBE() {
>
> Could use variant here I think with a non empty collection, and see if it
> still throws IOOBE.
Done.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/751#discussion_r1155152703
PR Review Comment: https://git.openjdk.org/jfx/pull/751#discussion_r1155153010
PR Review Comment: https://git.openjdk.org/jfx/pull/751#discussion_r1155152937
