RFR: 8234773: Fix ThreadsSMRSupport::_bootstrap_list

[David Holmes]

On 4/01/2021 10:09 pm, Kim Barrett wrote:
>> 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.

Then I confess I do not understand enough about the nuances of C++ 
construction/copying semantics to know what this "direct initialization" 
is actually about.

David
-----

> 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
> 
>