On Tue, 15 Aug 2023 22:21:15 GMT, Jose Pereda <jper...@openjdk.org> wrote:
>> I removed it because my fix requires that `toArray` works correctly. The
>> easiest way to get a correctly working version is to extend `AbstractSet`,
>> which provides a default implementation that works correctly. As I think
>> the default implementation is good enough and performs well enough, I saw no
>> reason to fix the broken version.
>
> 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.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1076#discussion_r1295226655
