On Thu, 19 Oct 2023 15:04:59 GMT, Andy Goryachev <ango...@openjdk.org> wrote:
>> 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. created * [JDK-8318842](https://bugs.openjdk.org/browse/JDK-8318842) Node.lookupAll to use Set instead of List (Enhancement) ------------- PR Review Comment: https://git.openjdk.org/jfx/pull/1245#discussion_r1373382130
