On Mon, 8 Jan 2024 19:24:14 GMT, Andy Goryachev <ango...@openjdk.org> wrote:
>> I am confused, and maybe I am missing something. As far as I know, this >> method is not called anywhere. I put a breakpoint in it. JavaFX does not use >> this method anywhere, nor are Selectors ever used as keys in Sets or Maps. >> >> What am I missing? > > you are right, this code does not affect performance (I could not hit a break > point here either). > still, since you are touching these lines, why not do it right? I can undo the rename so they're not touched. My problem with fixing this is that I then also should do it for `CompoundSelector`, which is just completely unrelated to the intent of this PR. I'm pretty sure that within JavaFX we don't change lines that are unrelated to the intent of the change (these lines were only hit because of a clarifying field rename related to this change). Normally I would do such fixes in an extra commit, where the first commit would be something like "fix: Improve selector match performance" and the second is "fix: Fix bug in Selectors hashCode algorithm"... ... but since everything is squashed as the way of working in FX (instead of having stand alone commits that are fast forwarded when included in master), it would have to be done the official way, which is file a ticket, create a new PR, have it also reviewed, etc. This puts an additional burden on the already long review queue, with well intentioned solid PR's that are abandoned because of lack of feedback and reviews... Even simple things like fixing warnings I've given up on as I see it taking time away from more pressing issues. So, I can file a ticket and create another PR. ------------- PR Review Comment: https://git.openjdk.org/jfx/pull/1316#discussion_r1449489869
