On Wed, 16 Aug 2023 14:16:23 GMT, John Hendrikx <jhendr...@openjdk.org> wrote:
>> Thanks for that test, actually it could be used as part of a test in >> PseudoClassTest, to verify that the old implementation failed and the new >> one worked? >> >> And this brings up another issue: the constructor `PseudoClassState(List)` >> is not used anywhere, is it? Should it be removed then (unless it is added >> to such test)? > > Alright, I'll add the test. > > As for the other issue: there are quite a few other issues **near** the code > I've changed, but in order for a PR to remain focused, and not burdened with > changes that don't contribute to the intended change (making it smaller and > easier to review), it's best to address these in another PR that focuses > specifically on such problems (unused constructors, unused fields, unused > methods, etc). > > In this specific case, the constructor was not changed or added by me, it's > part of private API and it's not causing problems. Changing it would > distract from the goal of the PR, and so is IMHO out of scope. I agree this is out of scope. Once this PR is merged, there are many things that can be done to cleanup the code. ------------- PR Review Comment: https://git.openjdk.org/jfx/pull/1076#discussion_r1315170858
