RFR: JDK-8299688: Adopt C++14 compatible std::bit_cast introduced in C++20 [v8]

Kim Barrett kbarrett at openjdk.org
Sat Feb 4 22:52:52 UTC 2023


On Mon, 23 Jan 2023 19:10:50 GMT, Justin King <jcking at openjdk.org> wrote:

>> Adopts a C++14 compatible implementation of [std::bit_cast](https://en.cppreference.com/w/cpp/numeric/bit_cast).
>> 
>> `PrimitiveConversions::cast` is refactored into `bit_cast` and extended to support all compatible types. Additionally it makes use of `__builtin_bit_cast` when available, which is strictly well defined compared to fallback approaches which are sometimes lurking in undefined behavior territory.
>> 
>> Lastly some legacy casting is updated to use `bit_cast` in `utilities/globalDefinitions.hpp`.
>
> Justin King has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - Documentation updates
>    
>    Signed-off-by: Justin King <jcking at google.com>
>  - Restore comment regarding integral casting
>    
>    Signed-off-by: Justin King <jcking at google.com>

Changes requested by kbarrett (Reviewer).

.github/workflows/main.yml line 238:

> 236:     with:
> 237:       platform: macos-x64
> 238:       xcode-toolset-version: '12.5.1'

This and the other toolchain update proposed here are to avoid the
mentioned bug in Apple Clang, so that `__builtin_bit_cast` can be used
with that compiler.  Since I don't think we should be using the
builtin, I don't think this is needed either.  There may be other
reasons to move the GHA toolchain version forward, but that should be
a separate discussion.

src/hotspot/share/utilities/bitCast.hpp line 35:

> 33: // Casts from type From to type To without changing the underlying bit representation. This is
> 34: // partially compatible with std::bit_cast introduced in C++20, but is more restrictive on the type
> 35: // of conversions allowed.

If it's not compatible (or at least trying to be, in so far as possible within the restrictions we operate under)
with `std::bit_cast` (and I've suggested there are reasons why we shouldn't be compatible), then using the
name `bit_cast` seems like a mistake that will only cause confusion.

`PrimitiveConversions::cast` might not be the best name either (John Rose certainly expressed dislike), but
discussing a name change is probably better done outside the context of a PR.

In the meantime, I don't think this part of the change should be made.  There are various places that should
be using PrimitiveConversions::cast (under whatever name is ultimately chosen) but aren't, and they can be
changed to do so.  Later, once we've picked the final name, we can do a global rename.

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

PR: https://git.openjdk.org/jdk/pull/11865


More information about the hotspot-dev mailing list