On Tue, 15 Aug 2023 23:05:38 GMT, John Hendrikx <[email protected]> wrote:
>> The original ("broken") version has been working fine, and no bugs have been
>> reported so far, and there would be a reason to have a custom implementation
>> instead of the one in `AbstractSet` in the first place.
>>
>> I'm not against removing it, but only after we are certain that this
>> implementation is no longer needed.
>>
>> Also, have you tried fixing it instead of removing it? If you have, are
>> there any differences when you run the test with one or the other?
>
> 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)?
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1076#discussion_r1295883206
