RFR: 8314258: checked_cast doesn't properly check some cases
Kim Barrett
kbarrett at openjdk.org
Mon Oct 16 01:29:32 UTC 2023
Please review this improvement to the `checked_cast` utility.
checked_cast was added by JDK-8255544 to permit silencing of certain compiler
warnings (such as from gcc's -Wconversion) for narrowing conversions when the
value is "known" to be safely convertible. It provides debug-only runtime
verification that the conversion preserves the value while changing the type.
There has been a recent effort to apply checked_cast to eliminate -Wconversion
warnings, with the eventual goal of turning on such warnings by default - see
JDK-8135181.
The existing implementation checks that the value is unchanged by a round-trip
conversion, and has no restrictions on the arguments. There are several
problems with this.
(1) There are some cases where conversion of an integral value to a different
integral type may pass the check, even though the value isn't in the range of
the destination type.
(2) Floating point to integral conversions are often intended to discard the
fractional part. But that won't pass the round-trip conversion test, making
checked_cast mostly useless for such conversions.
(3) Integral to floating point conversions are often intended to be
indifferent to loss of precision. But again, that won't pass the round-trip
conversion test, making checked_cast mostly useless for such conversions.
This change to checked_cast supports integral to integral conversions, but not
conversions involving floating point types. The intent is that we'll use
"-Wconversion -Wno-float-conversion" instead of -Wconversion alone. If/when
we later want to enable -Wfloat-conversion, we can either extend checked_cast
for that purpose, or probably better, add new functions tailored for the
various use-cases.
It also supports enum to integral conversions, mostly for compatibility with
old code that uses class-scoped enums instead of class-scoped static const
integral members, to work around ancient broken compilers. We still have a
lot of such code.
This new checked_cast ensures (in debugging builds) that the value being
converted is in the range of the destination type. It does so while avoiding
tautological comparisons, as some versions of some compilers may warn about
such. Note that this means it can also be used to suppress -Wsign-conversion
warnings (which are not included in -Wconversion when compiling C++), which we
might explore enabling in the future.
It also verifies a runtime check is needed, producing a compile-time error if
not. Unnecessary checked_casts make the code harder to understand. This will
alert a developer that a change is rendering a checked_cast unnecessary, so it
can be removed. This compile-time check can be suppressed on a per-call
basis, as there are cases where a runtime check might only sometimes be needed.
Aside: Using C++17 if-constexpr would eliminate the metaprogramming needed to
implement the tautological comparison avoidance and the unnecessary
checked_cast error. The resulting implementation would be less than 1/3 the
size from this proposal, and easier to understand. Maybe later...
This change removes a small number of unnecessary checked_casts, found by the
check for such. It also suppresses that check in a few call sites where
that's needed. Finally, it removes a call involving floating point.
Testing:
New gtests for checked_cast.
mach5 tier1-5
OpenJDK GHA Sanity Checks
-------------
Commit messages:
- remove float checked_cast
- suppress tautological conversion failures
- improved checked_cast
- remove unneeded checked_casts
Changes: https://git.openjdk.org/jdk/pull/16005/files
Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=16005&range=00
Issue: https://bugs.openjdk.org/browse/JDK-8314258
Stats: 397 lines in 10 files changed: 374 ins; 0 del; 23 mod
Patch: https://git.openjdk.org/jdk/pull/16005.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/16005/head:pull/16005
PR: https://git.openjdk.org/jdk/pull/16005
More information about the hotspot-dev
mailing list