On Wed, 26 Mar 2025 16:39:27 GMT, Jaikiran Pai <j...@openjdk.org> wrote:
>> 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. > > I just noticed that this `subgroupsAsArray()` private method is only used by > JVMTI and a comment exists about that on that method: > > > /** > * Returns a snapshot of the subgroups as an array, used by JVMTI. > */ > > So, irrespective of what solution we choose, perhaps we could add another > brief sentence to that comment, something like the following? > > > /** > * Returns a snapshot of the subgroups as an array, used by JVMTI. > * Care should be taken not to trigger any classloading in this method. > */ I switched to the code that does the array copy inline rather than rely on toArray(). I also added a comment. I intentionally kept is short and simple. I was tempted to say something about toArray(), reference the CR, mention JDI and the debug agent, etc, but that's heading down the "too wordy" path that I wanted to avoid. Readers can "git blame" to find out why it was done. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/24236#discussion_r2014778518
