RFR: JDK-8298528: Clean up raw type warnings in base in bindings and collections packages
Nir Lisker
nlisker at openjdk.org
Fri Dec 30 21:23:55 UTC 2022
On Sun, 11 Dec 2022 20:12:17 GMT, John Hendrikx <jhendrikx at openjdk.org> wrote:
> Packages fixed:
> - com.sun.javafx.binding
> - com.sun.javafx.collections
> - javafx.beans
> - javafx.beans.binding
> - javafx.collections
> - javafx.collections.transformation
Second part of the review. I'm still looking at the changes to sorting. I think that the problem stems from a more basic flaw that we can possible fix, and that is that any `ObservableListWrapper` is a `SortedList` even when it's not. A `SortedList` should require that its elements implement `Comparable`. The only other class affected is the `Sequential` variant, so the scope here is small.
While the current fix removes the warnings, I suspect we are just hiding the flaw that these warnings exist for.
In addition `FXCollections::sort(ObservableList<T>)` can change to
if (list instanceof SortableList<?> sortableList) {
sortableList.sort();
and remove the `@SuppressWarnings("unchecked")`.
modules/javafx.base/src/main/java/javafx/collections/FXCollections.java line 709:
> 707: private static class EmptyObservableList<E> extends AbstractList<E> implements ObservableList<E> {
> 708:
> 709: private static final ListIterator<Object> iterator = new ListIterator<>() {
Isn't a better fix is to not make this class static and use `<E>`? Other lists create a new iterator on each invocation, but this is effectively a singleton.
In fact, `EmptyObservableList` doesn't need to be declared because it's called only in one place, so it can be "inlined" when declaring the `EMPTY_LIST` field. Maybe this is out of scope.
modules/javafx.base/src/main/java/javafx/collections/FXCollections.java line 1640:
> 1638: @Override
> 1639: public Iterator<E> iterator() {
> 1640: return new Iterator<>() {
Here the empty `Set` creates a listener on invocation, unlike in the list case. Might want to keep a single pattern. I prefer the one with a singleton iterator because the empty set itself is a singleton. Same comment about considering "inlining" it.
modules/javafx.base/src/main/java/javafx/collections/transformation/TransformationList.java line 94:
> 92: List<?> currentSource = source;
> 93: while(currentSource instanceof TransformationList) {
> 94: currentSource = ((TransformationList<?, ?>)currentSource).source;
Can use pattern matching for `instanceof`.
Also, can add a space after `while`.
modules/javafx.base/src/main/java/javafx/collections/transformation/TransformationList.java line 143:
> 141: int idx = getSourceIndex(index);
> 142: while(currentSource != list && currentSource instanceof TransformationList) {
> 143: final TransformationList<?, ?> tSource = (TransformationList<?, ?>) currentSource;
Sam here.
-------------
PR: https://git.openjdk.org/jfx/pull/972
More information about the openjfx-dev
mailing list