RFR: 8313882: Fix -Wconversion warnings in runtime code [v2]

[Coleen Phillimore]

On Tue, 8 Aug 2023 21:57:17 GMT, Patricio Chilano Mateo <pchilanomate at openjdk.org> wrote:

>> Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Make bytecode.hpp index() functions return u2 not long.
>
> src/hotspot/share/runtime/os.cpp line 143:
> 
>> 141:     milliseconds_since_19700101 / milliseconds_per_microsecond;
>> 142:   const int milliseconds_after_second =
>> 143:     checked_cast<int>(milliseconds_since_19700101 % milliseconds_per_microsecond);
> 
> Not related to this change but seems milliseconds_per_microsecond should be named milliseconds_per_second.

That's easily fixed, and I've already changed those lines.

> src/hotspot/share/runtime/safepoint.cpp line 98:
> 
>> 96:                                              int initial_number_of_threads,
>> 97:                                              int threads_waiting_to_block,
>> 98:                                              int iterations) {
> 
> Maybe leave iterations as unsigned and change the caller instead?

If I change the caller: synchronize_threads, it returns the parameter iterations, so I'd have to change its callers.  Everything uses iterations here as an int, other than this parameter.

> src/hotspot/share/runtime/sharedRuntime.cpp line 2348:
> 
>> 2346:     double sum = 0;
>> 2347:     double weighted_sum = 0;
>> 2348:     for (int i = 0; i <= n; i++) { sum += (double)histo[i]; weighted_sum += (double)(i*histo[i]); }
> 
> So hist is a uint64_t*, why can't we make sum and weighted_sum just uint64_t?

I think there were less casts this way, since sum and weighted sum had to be turned into double more than histo[i], but it does make more sense to use int for summing the numbers, and cast to double for printing.

> src/hotspot/share/runtime/threadHeapSampler.cpp line 359:
> 
>> 357:          "double and uint64_t do not have the same size");
>> 358:   x = *reinterpret_cast<const uint64_t*>(&d);
>> 359:   const uint32_t x_high = (uint32_t)(x >> 32);
> 
> Can't we do a checked_cast here? The cast should be reversible with that shift value.

I think so.  I was afraid of sign extension so I just wanted to chop it off, also didn't seem necessary to check.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/15177#discussion_r1287801694
PR Review Comment: https://git.openjdk.org/jdk/pull/15177#discussion_r1287802613
PR Review Comment: https://git.openjdk.org/jdk/pull/15177#discussion_r1287805610
PR Review Comment: https://git.openjdk.org/jdk/pull/15177#discussion_r1287819364