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

Various cleanups: Impl and wrapper functions for vector algorithms #4146

Merged

Conversation

StephanTLavavej
Copy link
Member

First, vector_algorithms.cpp was defining functions within an unnamed namespace that were quasi-shadowing wrapper functions in <algorithm> and <xutility>:

template <class _Traits, class _Ty>
__declspec(noalias) size_t
__stdcall __std_count_trivial(const void* _First, const void* const _Last, const _Ty _Val) noexcept {

STL/stl/inc/xutility

Lines 97 to 98 in f392449

template <class _Ty, class _TVal>
__declspec(noalias) size_t __std_count_trivial(_Ty* _First, _Ty* _Last, const _TVal _Val) noexcept {

This was incredibly confusing, so I'm renaming the vector_algorithms.cpp functions to impl. (I say it was "quasi"-shadowing because we never dragged them into the same scope, but we should never use the same names for things with different signatures/purposes like this. There are specific scenarios where function overloading is fine - usually when the compiler will handle types/arity and there's no risk of confusion about "which one" we're getting - and this is not such a scenario, so we should use different names.)

Next, this moves <algorithm> and <xutility>'s vector algorithm wrapper templates (that handle 1/2/4/8-byte dispatching) from the global namespace to std. I noticed this while auditing the STL for namespace discipline - aside from extern "C" machinery in the global namespace, the STL is almost always careful to define all of its internal machinery within std. (There are a few special exceptions for bitmask ops.) These wrapper templates were a major exception. I'm keeping their names, since they do strongly resemble the extern "C" functions they're wrapping, but we should move them into std for cleanliness (and because this may make later work more convenient for me).

@StephanTLavavej StephanTLavavej added the enhancement Something can be improved label Nov 3, 2023
@StephanTLavavej StephanTLavavej requested a review from a team as a code owner November 3, 2023 21:05
@StephanTLavavej StephanTLavavej self-assigned this Nov 6, 2023
@StephanTLavavej
Copy link
Member Author

I am a greedy kitty who's speculatively mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

@@ -9848,7 +9850,7 @@ namespace ranges {
if (!_STD is_constant_evaluated()) {
const auto _First_ptr = _STD to_address(_First);
const auto _Last_ptr = _First_ptr + (_Last - _First);
const auto _Result = _CSTD __std_minmax_element(_First_ptr, _Last_ptr);
const auto _Result = _STD __std_minmax_element(_First_ptr, _Last_ptr);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we remove _std_ from these function names - possibly in a followup - to avoid the awkward _STD __std?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea is that they behave just like the __std_meow_N functions without the N size; renaming them would break that symmetry. That said I wouldn't grumble too much, as long as the names are unique.

(If I had my way there wouldn't be any _STD at all and we wouldn't care about the incomplete type scenario 😹)

@@ -94,6 +94,7 @@ const void* __stdcall __std_max_element_4(const void* _First, const void* _Last,
const void* __stdcall __std_max_element_8(const void* _First, const void* _Last, bool _Signed) noexcept;
_END_EXTERN_C

_STD_BEGIN
template <class _Ty, class _TVal>
__declspec(noalias) size_t __std_count_trivial(_Ty* _First, _Ty* _Last, const _TVal _Val) noexcept {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto _STD __std_.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Something can be improved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants