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