RFR: 8314114: Fix -Wconversion warnings in os code, primarily linux [v5]

Thomas Stuefe stuefe at openjdk.org
Sat Aug 12 06:57:58 UTC 2023


On Fri, 11 Aug 2023 20:40:28 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

>> This change fixes various -Wconversion warnings from files in the os directories and related, widening types or adding checked_cast<> where narrowing.
>> Tested with tier1 on linux/macosx/windows and tier1-4 on linux-x64-debug, windows-x64-debug.
>
> Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Put back size_t parameter for print_signal_handlers and add a checked_cast<> future could fix the dll_address_to_x functions.

Hi Coleen,

I had a read through these changes. Remarks inline.

This is an immense work, respect.

When and where I had been grumbling, it was because whatever we change must be maintainable in the future. So, if we now do X in various places, X must not be arbitrary but be a consistent rule we can follow in the future, lest all this work bitrots quickly.

One proposal, possibly for a future RFE: could we maybe make checked_cast a smaller name? One gripe with C++ cast expressions I have is that they look ugly and give me RSI. Since this is our own name, and it is really omnipresent now, could we not shorten it (similar to macro names like "p2i")? For example `cc`. As in


size_t len
foo_takes_int(cc<int>(len))

or even better as a macro


foo_takes_int(cc(int, len))


Cheers, Thomas

src/hotspot/os/linux/os_linux.cpp line 3151:

> 3149:                            // are more reasonable) we'll just hardcode the number they use
> 3150:                            // in the library.
> 3151:   const int BitsPerCLong = (int)sizeof(long) * CHAR_BIT;

suggestion here and above: constexpr?

src/hotspot/os/linux/os_linux.cpp line 3352:

> 3350:   for (int index = 0; index < loops && !found_range; index ++) {
> 3351:     assert(pages > 0, "Nothing to do");
> 3352:     size_t pages_to_query = (pages >= stripe) ? stripe : pages;

Pre-existing: Could you instead make `pages` and `pages_to_query` and below `vecIdx` an uintx? Should be the same and give you no new errors, but size_t as "number of things" is not right.

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

PR Review: https://git.openjdk.org/jdk/pull/15229#pullrequestreview-1574842059
PR Review Comment: https://git.openjdk.org/jdk/pull/15229#discussion_r1292114968
PR Review Comment: https://git.openjdk.org/jdk/pull/15229#discussion_r1292115407


More information about the hotspot-dev mailing list