Replace all remaining instances of reinterpret_cast with std::bit_cast

[liuzicheng1987]

No description provided.

[GregTheMadMonk]

Sorry for interjecting, but isn't std::bit_cast designed to copy bits of one value as another value, and not to reinterpret pointers of different types?

cppreference doesn't have a single example of using it to replace reinterpret_cast on pointers. I'm no language lawer, but I have a feeling that in this context std::bit_cast is the same as reinterpret_cast and std::launder may still be needed

edit: bit_cast on pointers may even be ill-formed: isocpp/CppCoreGuidelines#1517 (comment)

[liuzicheng1987]

No, std::launder will not be needed for std::bit_cast. The reason that you need std::launder for reinterpret_cast is that reinterpret_cast is nothing more than an instruction to the compiler to ignore the type system for a moment. But the compiler is still free to perform optimizations that rely on the assumption that the thing pointed to is what it used to be rather than the reinterpretation.

By contrast, std::bit_cast is a much more invasive operation that prohibits such optimizations.

In fact, we have had a weird problem, where std::launder in combination with reinterpret_cast led to weird behaviour, but only for GCC 12 and only under certain compiler settings. These problems went away when we started using std::bit_cast in these places.

[liuzicheng1987]

The comment you are quoting only refers to instances where things are being cast as a void pointer, something you should never do.

[spectras]

The crux is std::bit_cast only copies the underlying representation.

In the suggested change, it means it creates a const T*, a T* or a const Error* by copying the value representation of an unsigned char*. There is no guarantee that the representation of an unsigned char* is identical to the representation of a const T*, a T* or a const Error* for the same memory location.

It is not even guaranteed that the value representation of an unsigned char* be a valid value representation of a const T*, T* or const Error*. And if it is not, the bit_cast yields undefined behavior.

By contrast, reinterpret_cast performs implementation-defined conversion. One could make this mental model of what happens when passing a pointer through those:

• reinterpret_cast changes the interpretation of the pointed object. To do so, reinterpret_cast might convert the representation to ensure the new pointer points to the same location.
• bit_cast changes the interpretation of the pointer itself. That is, bit_cast preserves the bit representation, which may be interpreted as a different location, or even be entirely invalid.

A (very unlikely but conforming afaik) implementation could use little-endian for some pointers and big-endian for others. In that case, reinterpret_cast would byte-swap when casting pointers, and bit_cast would not.

[liuzicheng1987]

@spectras, the problem is that in theory std::launder(reinterpret_cast(...)) should do the trick, but in practice we have had issues.

Specifically, we had a tuple implementation that used std::launder(reinterpret_cast(ptr + i)).

For GCC 12 (and only GCC 12, other compilers and GCC versions were fine) this led to a weird bug under some very specific compiler flags. Basically, ptr + i was transformed to just i.

The problem could be solved by simply removing std::launder or by using std::bit_cast. I am fairly certain that this is in fact a bug in GCC 12.

So how do you deal with this? In theory, std::launder(reinterpret_cast(...)) should be fine. In practice, it is not, but std::bit_cast is.

[liuzicheng1987]

@spectras and @GregTheMadMonk , what are your thoughts on this solution?

template <class T1, class T2>
inline T1 ptr_cast(T2* _ptr) {
  return static_cast<T1>(static_cast<void*>(_ptr));
}

template <class T1, class T2>
inline T1 ptr_cast(const T2* _ptr) {
  return static_cast<T1>(static_cast<const void*>(_ptr));
}

...

T& get_t() noexcept { return *ptr_cast<T*>(t_or_err_.data()); }

const T& get_t() const noexcept {
  return *ptr_cast<const T*>(t_or_err_.data());
}

Error& get_err() noexcept {
  return *ptr_cast<Error*>(t_or_err_.data());
}

const Error& get_err() const noexcept {
  return *ptr_cast<const Error*>(t_or_err_.data());
}

In my view this most certainly isn't UB, because it is never UB to static_cast as pointer to a void* and it is never UB to static_cast a void* to some other type.

[liuzicheng1987]

I am closing this in favour of #298.