Skip to content
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
merged 9 commits into from
Apr 9, 2024

Conversation

StephanTLavavej
Copy link
Member

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 and mutex.cpp via xthreads.h).


In _Cnd_internal_imp_t, rename its _Aligned_storage_t data member 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.


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 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.


Replace calls: cond->_get_cv() => Concurrency::details::_Get_cond_var(cond)


Move the definition of _Cnd_internal_imp_t from primitives.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 to static.

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).

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.
This expresses "16 or 8 bytes" as `2 * sizeof(void*)`.
@StephanTLavavej StephanTLavavej added the enhancement Something can be improved label Mar 31, 2024
@StephanTLavavej StephanTLavavej requested a review from a team as a code owner March 31, 2024 16:24
@StephanTLavavej StephanTLavavej self-assigned this Apr 8, 2024
@StephanTLavavej
Copy link
Member Author

I'm speculatively mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

stl/src/primitives.hpp Show resolved Hide resolved
@StephanTLavavej StephanTLavavej merged commit 29b30df into microsoft:main Apr 9, 2024
35 checks passed
@StephanTLavavej StephanTLavavej deleted the condition-variable branch April 9, 2024 20:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Something can be improved
2 participants