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

David Holmes dholmes at openjdk.org
Fri Aug 11 02:42:28 UTC 2023


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

>> src/hotspot/share/runtime/os.hpp line 781:
>> 
>>> 779:   static void print_siginfo(outputStream* st, const void* siginfo);
>>> 780:   static void print_signal_handlers(outputStream* st, char* buf, int buflen);
>>> 781:   static void print_date_and_time(outputStream* st, char* buf, size_t buflen);
>> 
>> I understand you are trying to minimize changes, but these functions all used size_t consistently.  It seems odd to change the type of just this one function.
>
> The reason I narrowed the types is because none of the callers pass size_t and widening dll_address_to_library_name requires platform dependent changes, and also changing the Decoder::decode() functions (or adding checked_casts<> somewhere along the way) and its subtypes.
> It's not simply to minimize changes, but it is.  It's to minimize casting, or checked_casting which is also not pleasant to look at in the code everywhere.  This is why I chose passing int as buflen.  There's other functions that pass int as buflen.
> Also, there's no world where we pass in a buffer that's greater than 32,000.  It's used for error reporting.

I counted (via grep) 62 occurrences of `int buflen` and 47 occurrences of `size_t` buflen in the hotspot code. So while it does look odd to have two different forms right next to each other, there is at least established precedent for each.

Unfortunately these `-Wconversion` changes in general seem the antithesis of code-cleanup and consistency as they force changes in limited contexts, resulting in inconsistencies unless we let the changes fan out to extreme degrees. I would have preferred in many cases to handle overflow/underflow via assertions rather than via the primitive type system. This quest for `-Wconversion` compliance is causing a great deal of pain - hats off to @coleenp for enduring that pain to put these PRs together.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/15229#discussion_r1290836647


More information about the hotspot-dev mailing list