Skip to content

vector_algorithms.cpp: introduce namespaces #5450

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

Open
wants to merge 26 commits into
base: main
Choose a base branch
from

Conversation

AlexGuteniev
Copy link
Contributor

@AlexGuteniev AlexGuteniev commented Apr 29, 2025

Resolves #5418

Why namespaces

The goals were:

  • Being able to locate functions using IDE navigation
  • Group related stuff to make it clearer what uses what

Both goals are achieved with partial success:

  • IDE navigation is way better, but still clobbered with now-empty unnamed namespaces. DevCom-10889095 is about that.
  • Grouping works, but some algorithms still use _Find_trats_ or find itself, now it happens across namespaces

Changes

  • Move functions so that the related are grouped and unrelated are ungrouped; each group contains its trait structs, implementation functions, and export functions
  • Separate traits from _Find_traits_ where makes sense
  • Rename existing namespaces and functions to match usual _Naming_style.
  • Add new namespaces
  • Rename some identifiers to shorter, as they now, in particular _Predicate
  • Move comments, change comment, introduce variables to resist silly wrapping
  • Remove some using namespace to make it more clear what is used from _Bitmap_details
  • Exclude some traits from _ARM64_EC and replace by void or completely remove

Changes are incremental to enable per-commit review.
(It may be complex to review the whole change, as something is both moved and modified)

Algorithms and namespaces

Namespaces:

  • _Bitset_from_string, _Bitset_to_string: contain corresponding bitset member functions implementations. These were fine, just renamed.
  • _Sorting: minmax_element, minmax, is_sorted_until. These have obvious commonalities, and even referred by the Standard with this name.
  • _Removing: remove, unique, remove_copy, unique_copy: They do the same shuffle and share tables for it.
  • _Mismatch: only mismatch itself. Originally was among find-like algorithm due to sharing traits. Decided to move out as it does not resemble find, and re-implmenet similar traits separately, which only need some functions from find traits. The use of traits can be completely avoided for this algorithm, but that's another story.
  • _Count: only count itself. As originally implemented, it was seen as find-like algorithm, however later it was re-implemented with a dedicated approach. The traits are its own, but still inherit from _Find_traits_, as all functions from them are needed.
  • _Find_meow_of: find_first_of and basic_string members that match against a set of characters. Pre-existing namespace, but reorganized a bit, in particular, some using namespace removed.
  • _Find_seq: search, find_end, and basic_string members members that match subsequence. These are distinct enough from _Find in that they use SSE4.2 like _Find_meow_of. They still use _Find_traits_, but only for fallbacks. Although if there would ever be 32 and 64 bit implementation, it will likely use _Find_traits_ fully.
  • _Find: find, find_last, adjacent_find, search_n, and basic_string members members that match one character, including single-chararcter find_meow_not. They have also much of commonality. search_n stands out a bit, but it still uses the same traits, and generally matches a string against a single character, so that is probably fine to keep it here.
  • _Reversing: reverse, reverse_copy. The namespace only contains common fallback

The algorithms that don't have traits or common functions don't need namespaces:

  • swap_ranges
  • replace

Though I'm sure it is now better with more structure, I'm not sure in particular namespaces usage.

@AlexGuteniev AlexGuteniev requested a review from a team as a code owner April 29, 2025 15:27
@github-project-automation github-project-automation bot moved this to Initial Review in STL Code Reviews Apr 29, 2025
@StephanTLavavej StephanTLavavej added the enhancement Something can be improved label Apr 29, 2025
@StephanTLavavej StephanTLavavej self-assigned this Apr 29, 2025
@StephanTLavavej
Copy link
Member

Thanks! I really appreciate the well-structured fine-grained commits, as this would have been impossible to review otherwise. 😻 I pushed a few more commits:

  • Drop extra newlines.
  • I took this opportunity to make our _M_ARM64EC comments consistent:
    • Arrow comments: !defined(_M_ARM64EC) => ^^^ !defined(_M_ARM64EC) ^^^
    • Arrow comments: !_M_ARM64EC => ^^^ !defined(_M_ARM64EC) ^^^
  • _Find_last => _Find_last_impl for consistency.
    • This was the only one that was missing _impl.
  • Add missing __stdcall when signatures exactly match for one-line returns.
    • This is only relevant for x86 and inlining might mean it doesn't make any difference in practice, but strictly following the pattern is easier to reason about.
  • Finally, some of the really short namespaces looked too much like functions, and _Count especially was very problematic, so I renamed them for consistency with _Removing, _Reversing, _Sorting:
    • namespace _Find => namespace _Finding
    • namespace _Count => namespace _Counting to avoid shadowing parameters!
    • namespace _Mismatch => namespace _Mismatching

@StephanTLavavej StephanTLavavej removed their assignment May 2, 2025
@StephanTLavavej StephanTLavavej moved this from Initial Review to Ready To Merge in STL Code Reviews May 2, 2025
@StephanTLavavej StephanTLavavej moved this from Ready To Merge to Merging in STL Code Reviews May 9, 2025
@StephanTLavavej
Copy link
Member

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

StephanTLavavej added a commit to StephanTLavavej/STL that referenced this pull request May 9, 2025
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
Status: Merging
Development

Successfully merging this pull request may close these issues.

vector_algorithms.cpp: split into multiple translation units or otherwise enhance file navigation?
2 participants