-
Notifications
You must be signed in to change notification settings - Fork 1.6k
<atomic>: drop _Atomic_reinterpret_as
#5973
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
base: main
Are you sure you want to change the base?
Conversation
| } else if constexpr (is_pointer_v<_Ty> && sizeof(_Integral) == sizeof(_Ty)) { | ||
| return reinterpret_cast<_Integral>(_Source); | ||
| } else { | ||
| _Integral _Result{}; // zero padding bits |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was misleading.
If _Ty has padding bits, this does zero the corresponding bits in _Integral, along with other bits.
But memcpy just below copies the values of these padding bits anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, after thinking about it a bit I can see it.
The intent was to make some non-power-of-two sized struct handled here. Like struct s { char a; char b; char c; };.
"Padding bits" is a misnomer; these are actually bytes to complete to integral type.
For now and until vNext we don't really have lock-free atomics for like struct s { char a; char b; char c; }. We may reconsider the current change to preserve this part, or defer this to vNext.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I admit to not having the best understanding on getting the compiler to zero out the correct bits but I believe I wrote tests that tried to introduce the infinite loop problem on bogus padding
045c67e to
6f8da55
Compare
The goal of the change is mostly clarity. Bit cast is a vocabulary function.
_Bit_castwas invented too late, otherwise_Atomic_reinterpret_asshould have been_Bit_castall along.In addition to size checks,
_Atomic_reinterpret_asalso check for integrality, but that's an arbitrary requirement we don't really need to enforce.Bit cast has some triviality check, which spotted unwrapped
_Storage_foron lines 927 and 1062 (944 and 1080 before the change), fixed as a separate commit.