On Thu, 17 Aug 2023 12:10:51 GMT, Coleen Phillimore <cole...@openjdk.org> wrote:
>> src/hotspot/share/logging/logOutput.cpp line 69:
>>
>>> 67:
>>> 68: static int tag_cmp(const LogTagType *a, const LogTagType *b) {
>>> 69: return primitive_compare(a, b);
>>
>> This looks very odd given we are dealing with pointers not primitives.
>
> Was it odd before or odd now? What we want to do is compare pointers for a
> sort function. This primitive_compare has been used in other places as an
> improvement.
It is the name `primitive_compare` - I only previously saw it used for integer
types. Using it with pointers seems "wrong". Don't we have to convert to
`intptr_t` to compare pointers numerically anyway?
>> src/hotspot/share/services/threadService.hpp line 107:
>>
>>> 105: static jlong get_peak_thread_count() { return
>>> _peak_threads_count->get_value(); }
>>> 106: static int get_live_thread_count() { return
>>> _atomic_threads_count; }
>>> 107: static int get_daemon_thread_count() { return
>>> _atomic_daemon_threads_count; }
>>
>> Given all the other jlong usage in these functions, and that these are used
>> for the return value of `jlong get_long_attribute(jmmLongAttribute att)` in
>> management.cpp, I think these should stay as jlong returning functions.
>
> If they stay jlong returns (note that these fields are in fact int), then we
> need to add casting to all the callers. Casting is worse than returning the
> correct types. If someone wants to make these fields jlong someday then they
> can propagate the change to the callers. This change corrects the types.
I don't follow. The fields are int so cast them to jlong before returning them.
All the callers of these methods expect jlong so there can't be any issue there.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/15233#discussion_r1297870904
PR Review Comment: https://git.openjdk.org/jdk/pull/15233#discussion_r1297871632
