On Fri, 31 Mar 2023 15:32:06 GMT, Michael Strauß <[email protected]> wrote:
>> John Hendrikx has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Simplify ImmutablePseudoClassSetsCache and avoid an unnecessary copy
>
> modules/javafx.graphics/src/main/java/com/sun/javafx/css/BitSet.java line 584:
>
>> 582: * @param obj the object to cast, cannot be {@code null}
>> 583: * @return a type T, or {@code null} if the argument was not of
>> this type
>> 584: * @throws NullPointerException when {@code obj} is {@code null}
>
> Previously, this method always returned an instance of `T`. Now that is not
> the case, it might also simply return `null` if the argument passed into it
> is an instance of a different class. I think it makes sense to also return
> `null` when the argument passed into the method is `null`.
I'm a bit unsure why that would be an improvement. Passing `null` to a function
that doesn't expect it should IMHO never just return `null` but should instead
be considered a programming error and result in a stack trace. Passing in a
non-null value that can't be casted is explicitly documented now that it would
result in `null`. One is a caller error, the other isn't IMHO (as the caller
can't check if it is castable without another method -- I considered adding an
`instanceof` method).
Or maybe I'm reading too much in to this and you are just pointing out that the
function has changed from its previous contract -- I think this is okay as
`BitSet` is not public API, nor are any of its subclasses.
> modules/javafx.graphics/src/main/java/com/sun/javafx/css/BitSet.java line 588:
>
>> 586: protected abstract T cast(Object obj);
>> 587:
>> 588: protected long[] getBits() {
>
> Since your patch already contains some cleanup work: can you make this method
> `final`? The way it's specified at the moment looks like it was made to be
> overridable, which is clearly not useful.
Sure, I don't mind, the class is not public though, nor are its subclasses.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1076#discussion_r1154758868
PR Review Comment: https://git.openjdk.org/jfx/pull/1076#discussion_r1154758778
