-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
<condition_variable>
: Avoid squirrelly forward declaration of _Cnd_internal_imp_t
#4545
Merged
StephanTLavavej
merged 9 commits into
microsoft:main
from
StephanTLavavej:condition-variable
Apr 9, 2024
Merged
<condition_variable>
: Avoid squirrelly forward declaration of _Cnd_internal_imp_t
#4545
StephanTLavavej
merged 9 commits into
microsoft:main
from
StephanTLavavej:condition-variable
Apr 9, 2024
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This will allow us to move the definition of `_Cnd_internal_imp_t` into `__msvc_threads_core.hpp`. This is a safe transformation because only 3 files include `primitives.hpp` and they already drag in `__msvc_threads_core.hpp` (`sharedmutex.cpp` directly; `cond.cpp` and `mutex.cpp` via `xthreads.h`).
… `cv` to `_Cv_storage`. This will allow us to move the definition into `stl/inc/__msvc_threads_core.hpp`. The new name is consistent with how `_Mtx_internal_imp_t` names its `_Aligned_storage_t` data member `_Cs_storage`.
…tails::_Get_cond_var` Change the member function `_Cnd_internal_imp_t::_get_cv` into the non-member function `Concurrency::details::_Get_cond_var`, renamed to be more descriptive. Drop the unnecessary comment `// get pointer to implementation`. Now we don't need to qualify `stl_condition_variable_win7`, defined immediately above. We need to take a parameter `::_Cnd_internal_imp_t* _Cond`, qualified for clarity. We need `inline` for the non-member function.
… `__msvc_threads_core.hpp`. This is still within `extern "C"`. Change `std::` => `_STD`. We no longer need the wacky `/clr` forward declaration workaround.
We can do this because its only data member is exactly `_Aligned_storage_t<_Cnd_internal_imp_size, _Cnd_internal_imp_alignment>`, so the representation is unchanged. Then we can simplify the `_Mycnd()` member function. This follows the same pattern as `_Mutex_base`'s `_Mtx_internal_imp_t _Mtx_storage` and `_Mymtx()`.
…Cnd_internal_imp_t`. This changes `_INLINE_VAR` to `static`. The comment `// Size and alignment for _Cnd_internal_imp_t` is now unnecessary and can be dropped.
… their only uses.
This expresses "16 or 8 bytes" as `2 * sizeof(void*)`.
I'm speculatively mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed. |
CaseyCarter
approved these changes
Apr 9, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Followup to #4457 (comment). I performed this as a series of well-structured commits.
Commits
primitives.hpp
: Include__msvc_threads_core.hpp
.This will allow us to move the definition of
_Cnd_internal_imp_t
into__msvc_threads_core.hpp
.This is a safe transformation because only 3 files include
primitives.hpp
and they already drag in__msvc_threads_core.hpp
(sharedmutex.cpp
directly;cond.cpp
andmutex.cpp
viaxthreads.h
).In
_Cnd_internal_imp_t
, rename its_Aligned_storage_t
data membercv
to_Cv_storage
.This will allow us to move the definition into
stl/inc/__msvc_threads_core.hpp
.The new name is consistent with how
_Mtx_internal_imp_t
names its_Aligned_storage_t
data member_Cs_storage
.Change definition:
_Cnd_internal_imp_t::_get_cv
=>Concurrency::details::_Get_cond_var
Change the member function
_Cnd_internal_imp_t::_get_cv
into the non-member functionConcurrency::details::_Get_cond_var
, renamed to be more descriptive.Drop the unnecessary comment
// get pointer to implementation
.Now we don't need to qualify
stl_condition_variable_win7
, defined immediately above.We need to take a parameter
::_Cnd_internal_imp_t* _Cond
, qualified for clarity.We need
inline
for the non-member function.Replace calls:
cond->_get_cv()
=>Concurrency::details::_Get_cond_var(cond)
Move the definition of
_Cnd_internal_imp_t
fromprimitives.hpp
to__msvc_threads_core.hpp
.This is still within
extern "C"
.Change
std::
=>_STD
.We no longer need the wacky
/clr
forward declaration workaround.Directly store
_Cnd_internal_imp_t _Cnd_storage
.We can do this because its only data member is exactly
_Aligned_storage_t<_Cnd_internal_imp_size, _Cnd_internal_imp_alignment>
, so the representation is unchanged.Then we can simplify the
_Mycnd()
member function.This follows the same pattern as
_Mutex_base
's_Mtx_internal_imp_t _Mtx_storage
and_Mymtx()
.Move
_Cnd_internal_imp_size
/_Cnd_internal_imp_alignment
within_Cnd_internal_imp_t
.This changes
_INLINE_VAR
tostatic
.The comment
// Size and alignment for _Cnd_internal_imp_t
is now unnecessary and can be dropped.Fuse
_Critical_section_align
and_Cnd_internal_imp_alignment
into their only uses.Follow
_Cnd_internal_imp_t
's simpler pattern in_Mtx_internal_imp_t
.This expresses "16 or 8 bytes" as
2 * sizeof(void*)
.Note
After this, I believe that we should apply a similar bugfix as #4294, checking
UNDOCKED_WINDOWS_UCRT
when determining_Cnd_internal_imp_size
, but I'm not doing that here (mixing functional changes with cleanups is bad).