RFR: 8252406: Introduce Thread::as_Java_thread() convenience function [v2]

[Kim Barrett]

> On Sep 8, 2020, at 3:00 AM, David Holmes <david.holmes at oracle.com> wrote:
> 
> On 8/09/2020 4:34 pm, Kim Barrett wrote:
>> […]

>> src/hotspot/share/runtime/thread.hpp
>>  507   const JavaThread* as_const_Java_thread() const;
>> This missed part of what I'd suggested. I don't think the "const" part
>> of the name is needed or helpful.  Just overload as_Java_thread on the
>> const-ness of the thread it's being applied to.
> 
> I saw that but thought it was a typo

Intentional, not a typo.

> as I don't think about overloads in terms of const or other modifiers. To me overloads are same named methods that differ in arguments (and I'm going to guess that 'this' is considered an argument in this context and hence leads to the overload based on the const?).

That's correct, the function's const qualifier or lack thereof provides
overloading on the corresponding qualification of the 'this' argument.
Such overloading is a "common C++ idiom".

> But in addition I wanted to have this say as_const_JavaThread because I wanted to clearly distringuish it from as_JavaThread because there is only a couple of special cases in the JFR code where this const version is needed. If that code were to change and is_const_JavaThread because unused, it would be easy to see that.

I think those cases are just jfr being better than typical HotSpot code
about const qualifications. I wouldn't be surprised if there were other
places that could benefit from having such an overload pair if we tried
harder to be const-correct. So I would prefer overloading rather than the
odd name.

>> The fannout from putting the conversion functions in thread.inline.hpp seems
>> pretty high in the number of files that get touched. Especially since it
>> leads to JavaThread::current() and JavaThread::as_CompilerThread() being
>> moved. I would have been at least fine with, and probably would prefer,
>> having the definitions of as_JavaThread near the old definitions of those
>> functions in thread.hpp.
> 
> I tried to do the right and obey the "use .inline.hpp for inline definitions" rule. But I must confess I was reaching a point where I thought this was going too far but it's a good exercise with using git and PRs so I don't mind if I back up and take a simpler approach - especially as I just discovered more build failures due to missing includes. :(

I don't think there is such a rule, but that's a deep discussion that goes
far beyond the scope of this change. I think there are problems with how we
are using "inline" files. Stefan Karlsson and I (and others?) had a long
discussion about this a while back, which I can't find right now (drat!).