On 8/09/2020 4:34 pm, Kim Barrett wrote:
On Sep 8, 2020, at 12:09 AM, David Holmes <dhol...@openjdk.java.net> wrote:
David Holmes has updated the pull request incrementally with one additional
commit since the last revision:
Used static_cast instead of raw C-style cast
Moved inline functions to thread.inline.hpp
Updated #includes to deal with the move to thread.inline.hpp
Updated copyright year where needed.
-------------
Changes:
- all: https://git.openjdk.java.net/jdk/pull/37/files
- new: https://git.openjdk.java.net/jdk/pull/37/files/b18faad5..da70f804
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk&pr=37&range=01
- incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=37&range=00-01
Stats: 102 lines in 33 files changed: 39 ins; 28 del; 35 mod
Patch: https://git.openjdk.java.net/jdk/pull/37.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/37/head:pull/37
PR: https://git.openjdk.java.net/jdk/pull/37
------------------------------------------------------------------------------
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 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?).
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.
There are some situations where there is such an overload pair and it
is useful to additionally have an explicit const function, but I don't
think this is one of those.
------------------------------------------------------------------------------
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. :(
Thanks,
David
------------------------------------------------------------------------------
