RFR: 8234773: Fix ThreadsSMRSupport::_bootstrap_list
Kim Barrett
kim.barrett at oracle.com
Mon Jan 4 12:09:04 UTC 2021
> On Jan 3, 2021, at 9:16 PM, David Holmes <dholmes at openjdk.java.net> wrote:
> […]
> Hi Kim,
> Overall the fix looks good. I have one comment on the direct initialization part (after reading up on direct initialization).
>
> Thanks,
> David
>
> src/hotspot/share/runtime/threadSMR.cpp line 79:
>
>> 77:
>> 78: // The bootstrap list is empty and cannot be freed.
>> 79: ThreadsList ThreadsSMRSupport::_bootstrap_list{0};
>
> To anyone unfamiliar with the details of direct initialization this is very obscure. I don't know how to interpret this given a ThreadsList has four member fields. Wouldn't `{}` have the same effect of implicitly zero-initializing all the fields?
> Stylistically I find this syntax jarring, an equals sign in there would look far more natural to me. :(
ThreadsList is not an aggregate; it has a constructor - `ThreadsList(int)`.
The proposed change calls that 1-arg constructor with an argument of 0.
Are you saying you would prefer
ThreadsList ThreadsSMRSupport::_bootstrap_list = { 0 };
This is copy-list-initialization, again because `ThreadsList` is not an
aggregate. In this particular case, it would end up doing the same thing as
direct-initialization, i.e. call the 1-arg constructor.
However, if that constructor were `explicit`, the initialization is
ill-formed and compilation should fail. And that constructor ought to be
`explicit`. HotSpot code is unfortunately *really* bad about not using
`explicit` where it ought to be used though.
Since I'm touching code in this area and have been reminded of it, I'm
planning to change the constructor to be `explicit`.
> Marked as reviewed by dholmes (Reviewer).
Thanks.
>
> PR: https://git.openjdk.java.net/jdk/pull/1921
More information about the hotspot-runtime-dev
mailing list