Improve and expose bit_cast implementation #535
Conversation
Improve the bit_cast implementation to be as close as the C++20 spec. Move it to lib/fizzy/cxx20/bit.hpp to match the <bit> header in C++20 and be available in the fizzy library.
Codecov Report
@@ Coverage Diff @@
## master #535 +/- ##
=======================================
Coverage 98.72% 98.73%
=======================================
Files 56 58 +2
Lines 8660 8681 +21
=======================================
+ Hits 8550 8571 +21
Misses 110 110 |
| typename std::enable_if_t<sizeof(To) == sizeof(From) && std::is_trivially_copyable_v<From> && | ||
| std::is_trivially_copyable_v<To>, | ||
| To> | ||
| bit_cast(const From& src) noexcept |
There was a problem hiding this comment.
Are you planning to use this outside of the tests?
There was a problem hiding this comment.
In C API and execute() implementation.
| @@ -7,6 +7,7 @@ add_library(fizzy::fizzy ALIAS fizzy) | |||
|
|
|||
| target_sources( | |||
| fizzy PRIVATE | |||
| cxx20/bit.hpp | |||
There was a problem hiding this comment.
Are we going to move span here too?
There was a problem hiding this comment.
Yes. We briefly discussed that on Telegram with @gumb0 some time ago.
test/unittests/bit_cast_test.cpp
Outdated
| using namespace fizzy; | ||
| using namespace testing; | ||
|
|
||
| // Test cases from https://en.cppreference.com/w/cpp/numeric/bit_cast#Example. |
There was a problem hiding this comment.
Is it supposed to work between different signs, e.g uint64<->int64? If so, can you add an example? If not can you add a static_assert in the impl?
There was a problem hiding this comment.
Yes, it should work for types of different signness. Also for arrays, structs and other C-like types.
| To> | ||
| bit_cast(const From& src) noexcept | ||
| { | ||
| static_assert(std::is_trivially_constructible_v<To>); // Additional, non-standard requirement. |
There was a problem hiding this comment.
https://en.cppreference.com/w/cpp/numeric/bit_cast#Example has actually this line, do you mean that the final standard does not require it? I guess it has no issues as long as we keep compiling with <C++20 any such usage will be detected in our codebase, if we want to avoid such usage.
There was a problem hiding this comment.
Yes. The C++20 version only requires the conditions from std::enable_if. Personally, I would just use static_assert instead of std::enable_if, but if C++20 defines it like that let it be. I would also omit the is_trivially_constructible_v check, but being more pedantic is ok I guess.
| TEST(bit_cast, uint32_to_array) | ||
| { | ||
| std::array<uint8_t, 4> bytes; | ||
| bytes = bit_cast<decltype(bytes)>(uint32_t{0xaabbbbaa}); |
There was a problem hiding this comment.
could be non-symmetric to see the endianness
There was a problem hiding this comment.
It's symmetric to avoid endianness issues.
|
|
||
| #include <type_traits> | ||
|
|
||
| namespace fizzy |
There was a problem hiding this comment.
Oh actually could have added the C++20 check as in span.hpp, to test this with a C++20 compiler.
Improve the bit_cast implementation to be as close as the C++20 spec.
Move it to lib/fizzy/cxx20/bit.hpp to match the header in C++20
and be available in the fizzy library.