RFR: 8234773: Fix ThreadsSMRSupport::_bootstrap_list

[David Holmes]

Follow up ...

On 4/01/2021 11:08 pm, David Holmes wrote:
> 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.

I should avoid emailing late in the evening :)

The use of the "{ 0 }" syntax confused me because in my reading of:

https://en.cppreference.com/w/cpp/language/direct_initialization

it seemed to me that that syntax was used for non-class types, but 
apparently that is not the case.

Using:

ThreadsList ThreadsSMRSupport::_bootstrap_list(0);


as given in the bug report would have been much clearer to me as it more 
obviously looks like a constructor invocation.

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

I am not familiar with use of "explicit" so can't comment on that aspect.

Thanks,
David
-----

>>
>>> Marked as reviewed by dholmes (Reviewer).
>>
>> Thanks.
>>
>>>
>>> PR: https://git.openjdk.java.net/jdk/pull/1921
>>
>>