On Wed, 18 Oct 2023 18:07:53 GMT, Andy Goryachev <[email protected]> wrote:
>> Sai Pradeep Dandem has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8185831: Fixed test failing issues and code review comments
>
> modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 1993:
>
>> 1991: */
>> 1992: List<Node> lookupAll(Selector selector, List<Node> results) {
>> 1993: if (selectorMatches(selector)) {
>
> line 1990 might be incorrect: the return value **can** be null.
> We probably should just correct the javadoc, and since it's a private API no
> CSR is needed. All callers of this method do check for null.
I must say I'm again baffled at how this is implemented.
Red flag one: it uses a `LinkedList` which are known to be rarely the right
choice, and I doubt this case is any different.
Red flag two: a `List` is converted to a `Set`; being backed by a `LinkedList`
means set type operations will be terribly slow if that list has any kind of
size.
Red flag three: The choice to return a `Set` in the public API seems to be only
motivated to indicate there won't be any duplicates, which is a terrible reason.
Red flag four: `UnmodifiableListSet` talks about insertion speed and hashing
overhead, yet uses a `LinkedList` which are slower than `ArrayList` when it
comes to insertion speed (not to mention that they require more object
allocations and more memory(!)).
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1245#discussion_r1364940463
