On Tue, 15 Aug 2023 14:35:33 GMT, Jose Pereda <jper...@openjdk.org> wrote:
>> It's definitely a bug, but it would only appear if you had bit sets which >> required more than 1 long to store their data **and** you called `toArray`. >> It would then throw an `ArrayIndexOutOfBoundsException` in this line: >> >> final long state = getBits()[index]; >> >> This is because `index` is used for two unrelated purposes: >> - the index into the array of longs >> - the index of the entry in the resulting array >> >> If the outer loop ever needs two passes, it again wants to read a `long` >> using `getBits()[index]` but as `index` was advanced (multiple times) while >> filling entries in the result array, it won't be the correct index at all. >> >> Anyway, I'm not sure what the value would be of a test. Since `toArray` is >> no longer implemented by us but by `AbstractSet`, I'd just be testing the >> `AbstractSet` code. > > Okay, thanks for the clarification. > > So if I get it right, the removal of `toArray` doesn't have to do with the > bug, but with the fact that with the immutable set it is not longer required? 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. ------------- PR Review Comment: https://git.openjdk.org/jfx/pull/1076#discussion_r1295021835
