On Thu, 11 Jan 2024 22:59:33 GMT, Andy Goryachev <ango...@openjdk.org> wrote:
>> 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. I agree with John that changing the existing hash algorithm is really out of scope for this PR (even if it is suboptimal, which it is). Filing a separate bug has the advantage of decoupling it in time as well as not burdening this review. It may or may not even be worth implementing the optimization if no one uses it, but in any case, it can be done later. ------------- PR Review Comment: https://git.openjdk.org/jfx/pull/1316#discussion_r1449506858
