> On Sep 8, 2020, at 3:00 AM, David Holmes <david.hol...@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!).
