On Sat, 19 Sep 2020 05:18:11 GMT, yosbits
<github.com+7517141+yos...@openjdk.org> wrote:
>> modules/javafx.base/src/main/java/com/sun/javafx/collections/ObservableListWrapper.java
>> line 177:
>>
>>> 175: beginChange();
>>> 176: boolean removed = false;
>>> 177: for (int i = size()-1; i>=0; i--) {
>>
>> 1. Use spaces between operators.
>> 2. Why reverse the iteration order?
>>
>> Implementation discussion:
>> * We can use `boolean removed = removeIf(c::contains);` instead of the loop.
>> However, this method uses the iterator and
>> not random access, which *could* be less performant. The class implements
>> `RandomAccess`, suggesting that this is the
>> case, however, I don't see why. It seems that the implementation can be
>> any list, such as `LinkedList`, which is not
>> `RandomAccess`.
>> * Why do we need to override the inherited behavior? The super
>> implementation, which is the one from
>> `ModifiableObservableListBase`, does
>> ```
>> beginChange();
>> try {
>> boolean res = super.removeAll(c);
>> return res;
>> } finally {
>> endChange();
>> }
>> ```
>> (except that it does not check for empty collections at the start - should
>> it?) The `super` here is
>> `AbstractCollection`, which does `Iterator`-based removal.
>>
>> Same goes for the other method.
>
> Since the purpose of this issue is performance tuning,
> Rewriting with removeIf has a different purpose.
> This is the same as the original code.
>
> The iteration order is reversed because index movement does not occur during
> deletion. This is the same as the original
> code.
wondering why the implementation with the BitSet was choosen at all? Certainly
feels like a detour, so what are the
corner cases that I'm too blind to see?
-------------
PR: https://git.openjdk.java.net/jfx/pull/305
