-
Notifications
You must be signed in to change notification settings - Fork 5.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Favor bit_cast over reinterpret_cast #1517
Comments
There are at least rules discouraging casts: https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#es48-avoid-casts, https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#es49-if-you-must-use-a-cast-use-a-named-cast. |
Editors call: Even though it's a C++20 features and the Guidelines don't yet support/require C++20, enough people are interested in this that we should say something. Assigning to @GabrielDosReis. Thanks! |
Just for my understanding, do I understand correctly that this pull request favors bit_cast over reinterpret_cast in any possible case? Including the cases depicted by three code examples below here?
I'm very interested to hear if that is indeed going to be the official guideline, with respect to reinterpret_cast versus bit_cast |
This might be il-formed because
It's not guaranteed that
Ditto for |
Thanks @jwakely This particular void* example is actually based on an attempt of mine to replace some C-style casts like the one from https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/beginthread-beginthreadex doing:
Would it be any better when doing
So again, would it be better to use reinterpret_cast here? |
Your reinterpret cast examples are ok, I would not replace any of them with |
@jwakely Thanks again, Jonathan. So do you have a suggestion how the core guideline on reinterpret_cast versus bit_cast should be like? Apparently it isn't simply "Favor bit_cast over reinterpret_cast"! |
Correct me if I'm wrong, but I thought using bit_cast as a 1:1 replacement for reinterpret cast ist almost always wrong.
|
Right. But that doesn't make much sense for Niels' examples, where he wants to read the |
Absolutely. My point is that |
On another, but related topic: Is my understanding correct that recent changes to the standard (don't remember the name unfortunately and whether it was c++20, c++23 or a DR) now officially permit to cast a pointer to a range of raw bytes (e.g. received over the network or mmap) to a pointer to a c++ object and dereference this pointer? |
@MikeGitb This might be the paper you are thinking of: cplusplus/papers#592 |
@sweemer : Thanks, but I don't think that is it. From crossreading that paper talks about viewing the storage of an What I'm talking aobut is the reverse case: I'm getting a pointer to a char/std::byte buffer that has been filled with bytes that contain a valid representation of a |
Given the |
I guess this usage sometimes requires |
Thats the paper I had in mind. Thank you very much. |
It appears that commit 548b45f ("BUG: Replace C-style casts from `_beginthreadex` with `bit_cast<HANDLE>`", from pull request InsightSoftwareConsortium#3380) was wrong: `bit_cast<HANDLE>` might do a different conversion than the corresponding C-style cast, `(HANDLE)`. The C-style cast `(HANDLE)` behaves exactly like `reinterpret_cast<HANDLE>`, by definition. `reinterpret_cast<HANDLE>` does an implementation-defined conversion, as was explained by Jonathan Wakely at isocpp/CppCoreGuidelines#1517 (comment) (issue "Favor bit_cast over reinterpret_cast"). Fixed by replacing all `bit_cast<HANDLE>` calls with `reinterpret_cast<HANDLE>`.
It appears that commit 548b45f ("BUG: Replace C-style casts from `_beginthreadex` with `bit_cast<HANDLE>`", from pull request #3380) was wrong: `bit_cast<HANDLE>` might do a different conversion than the corresponding C-style cast, `(HANDLE)`. The C-style cast `(HANDLE)` behaves exactly like `reinterpret_cast<HANDLE>`, by definition. `reinterpret_cast<HANDLE>` does an implementation-defined conversion, as was explained by Jonathan Wakely at isocpp/CppCoreGuidelines#1517 (comment) (issue "Favor bit_cast over reinterpret_cast"). Fixed by replacing all `bit_cast<HANDLE>` calls with `reinterpret_cast<HANDLE>`.
@jwakely Can you please confirm that the line of code that does a
Assuming that a bitwise conversion is intended, I guess that would be a proper use case for |
The reinterpret_cast itself is ok, but dereferencing the result of the cast is undefined. The bit_cast version is ok, but redundant. For two's complement integers the implicit conversion from |
@jwakely Ah, thanks Jonathan. I see now that with p0907R2 - Signed Integers are Two’s Complement, in case of out of range, the result is no longer implementation-defined. That's also new to me. So in this particular case (from With respect to my ITK pull request , I'm not entirely sure if all compilers supported by ITK already implement integer conversion exactly like that (without any disagreeing implementation-defined behavior), as ITK still supports C++14 compilation by GCC 5.1, LLVM Clang 3.4, and VS2017 (v141). So I think ITK might still use its own hand-written |
GCC, clang and msvc always implemented that rule on all supported architectures. That's why changing the standard was completely uncontroversial. It used to be implementation defined to allow for other representations, but no C++ compiler ever used anything except two's complement. |
@N-Dekker I think
But I also suggest you to use |
@N-Dekker @jwakely I agree with @frederick-vs-ja that the |
Now that
std::bit_cast
is coming in C++20, the valid use cases forreinterpret_cast
are slim to none in the majority of applications. I believe there should be a rule heavily discouragingreinterpret_cast
in favor ofstd::bit_cast
or another named cast.Often times switching from
reinterpret_cast
tostd::bit_cast
would be the difference between invoking undefined behavior or not, due to type aliasing rules.The text was updated successfully, but these errors were encountered: