On Wed, 26 Mar 2025 09:31:37 GMT, Alan Bateman <al...@openjdk.org> wrote:
>> src/java.base/share/classes/java/lang/ThreadGroup.java line 664:
>>
>>> 662: private ThreadGroup[] subgroupsAsArray() {
>>> 663: List<ThreadGroup> groups = synchronizedSubgroups();
>>> 664: return groups.toArray(new ThreadGroup[groups.size()]);
>>
>> Hello Chris, would a comment on this line be necessary to prevent future
>> (IDE encouraged [1]) refactoring of this code back to `toArray(new
>> ThreadGroup[0])`?
>>
>> [1]
>> <img width="983" alt="toArray"
>> src="https://github.com/user-attachments/assets/643e66b8-c18a-4a10-8961-2aa3b2869fcc";
>> />
>
> What would you think about using the simple code + loop from the JBS comment?
> That would remove any discussion about toArray and any concerns that it would
> load classes.
Regarding adding a comment, I wasn't too sure what the comment should say
because once you start down that path, it's hard to not end up with too much
detail, and then things just get very wordy. Since we have a test that will
fail if this is ever modified to cause classloading again, at least that should
help prevent undoing this fix. However, the one downside of the test is that I
can only get it to fail with non-debug builds, so it may not be caught early.
Regarding using the loop, that's probably a more bullet proof option, although
there is still no guarantee that someday someone won't look at it and say "I
can just use toArray() here". There's probably no escaping an AI code cleaning
bot coming to that conclusion sometime in the near future. Then we're back to
adding a comment to make sure it isn't changed.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/24236#discussion_r2014554610
