On Tue, 22 Sep 2020 07:35:49 GMT, yosbits <github.com+7517141+yos...@openjdk.org> wrote:
>> https://bugs.openjdk.java.net/browse/JDK-8253086 >> >> ObservableListWrapper.java >> * public boolean removeAll(Collection<?> c) >> * public boolean retainAll(Collection<?> c) >> >> These two methods use BitSet, but it doesn't make sense. >> By rewriting to the equivalent behavior that does not use BitSet, it is >> possible to reduce the CPU load in a wide range >> of use cases. >> The test is done with the following command. >> >> * gradle base: test >> * gradle controls: test > > yosbits has refreshed the contents of this pull request, and previous commits > have been removed. The incremental views > will show differences compared to the previous content of the PR. The fix looks good to me. I left a few comments on the test, but it looks like a great start. modules/javafx.base/src/main/java/com/sun/javafx/collections/ObservableListWrapper.java line 203: > 201: boolean retained = false; > 202: beginChange(); > 203: for (int i = size()-1; i>=0; i--) { Please fix the spacing. Per our coding guidelines, this should be: for (int i = size() - 1; i >= 0; i--) { modules/javafx.base/src/test/java/test/com/sun/javafx/collections/ObservableListWrapperTest.java line 38: > 36: import java.util.Arrays; > 37: import java.util.Collection; > 38: import java.util.Collections; Can you sort the imports? modules/javafx.base/src/test/java/test/com/sun/javafx/collections/ObservableListWrapperTest.java line 110: > 108: assertTrue(list.retainAll(Collections.EMPTY_SET)); > 109: assertEquals(0, list.size()); > 110: } Can you add a test for `retainAll` (in a separate test method) that removes multiple elements in a single call? Something like the following: * set list to `(1, 2, 3, 4, 5)` * remove `(2, 4)` * verify that the list equals `(1, 3, 5)` * remove `(1, 5)` * verify that the list equals `(3)` Similarly, a test for `retainAll` that removes multiple elements in a single call? * set list to `(1, 2, 3, 4, 5)` * retain `(1, 3, 5)` * verify that the list equals `(1, 3, 5)` * retain `(3)` * verify that the list equals `(3)` modules/javafx.base/src/test/java/test/com/sun/javafx/collections/ObservableListWrapperTest.java line 78: > 76: assertEquals(3, list.size()); > 77: assertTrue(list.removeAll(1)); > 78: assertEquals(2, list.size()); Can you add a check that the correct element was removed (i.e., check that the list equals `(2, 3)`)? ------------- PR: https://git.openjdk.java.net/jfx/pull/305
