On Wed, 18 Oct 2023 18:07:53 GMT, Andy Goryachev <ango...@openjdk.org> 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