On Thu, 11 Jan 2024 22:50:32 GMT, John Hendrikx <jhendr...@openjdk.org> wrote:
>> 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. I feel the same kind of frustration sometimes. The review process is indeed painfully slow. A different JBS/PR would be fine, I think, something along the lines of "suboptimal hashCode() implementation in ...". I also think it's ok to keep the new names (so you don't have to do extra work) since there will be a new PR. ------------- PR Review Comment: https://git.openjdk.org/jfx/pull/1316#discussion_r1449495652
