On Thu, 19 Oct 2023 05:47:35 GMT, John Hendrikx <jhendr...@openjdk.org> wrote:
>> 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(!)).
You are right, @hjohn - LinkedList seems like a bad choice! It should be
HashSet from the very beginning, shouldn't it?
But Set<Node> as a return value for lookups is correct, I think.
We could (should?) fix it in a separate PR.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1245#discussion_r1365705477
