On Wed, 16 Aug 2023 13:16:19 GMT, Jose Pereda <jper...@openjdk.org> wrote:
>> You could check this yourself if you want. The `BitSet` class had problems
>> in many places, was largely untested and, to be very honest, should never
>> have passed code review. It violated the `Set` contract almost everywhere,
>> which is problematic since sets backed by this `BitSet` class were exposed
>> in several places where they could be accessed by users.
>>
>> A simple test shows the problem (it doesn't throw an exception though, I
>> misread that):
>>
>> public static void main(String[] args) {
>> for (int i = 0; i < 65; i++) {
>> PseudoClassState.getPseudoClass("" + i);
>> }
>>
>> System.out.println(Arrays.asList(new PseudoClassState(List.of("0",
>> "1", "64")).toArray()));
>> }
>>
>> Prints:
>>
>> [0, null, null]
>>
>> There is no point in fixing the existing code; it won't perform any better,
>> but would require writing additional test cases to verify that an
>> implementation we don't need is doing what we can get for free.
>
> 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.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1076#discussion_r1295987373
