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

Expand std::iter_swap and remove _Swap_adl #3700

Merged
merged 6 commits into from May 18, 2023

Conversation

frederick-vs-ja
Copy link
Contributor

@frederick-vs-ja frederick-vs-ja commented May 14, 2023

Also

  • Move std::iter_swap from <utility> to <algorithm>.
  • In _Partition_by_median_guess_unchecked, only evaluate _Prev_iter(_Glast) once.

Edit: I attempted to remove _Swap_adl directly but this failed due to T::swap member functions.

Edit: I should write using _STD swap;.

Fixes #2692.

In WG21-N4140 and WG21-N4659, the std::swap overload for arrays relied on std::swap_ranges (N4140 [utility.swap]/6, N4659 [utility.swap]/6), while std::swap_ranges recursively relied on std::swap (N4140 [alg.swap]/2, N4659 [alg.swap]/2). So I believe even if old standards allowed specializing std::iter_swap with making its behavior different (which is not a fact IMO), std::swap shouldn't be affected.

std::iter_swap is only specified to be used for std::reverse (N4140 [alg.reverse]/2, N4659 [alg.reverse]/2, N4868 [alg.reverse]/2). As said in #2692 (comment), I think it should be OK to inline std::iter_swap in std::reverse even in old modes.

Also
- Move `std::iter_swap` from `<utility>` to `<algorithm>`.
- In `_Partition_by_median_guess_unchecked`, only evaluate `_Prev_iter(_Glast)` once.
@frederick-vs-ja frederick-vs-ja requested a review from a team as a code owner May 14, 2023 15:48
@github-actions github-actions bot added this to Initial Review in Code Reviews May 14, 2023
@frederick-vs-ja frederick-vs-ja changed the title Expand std::iter_swap and remove _Swap_adl Expand std::iter_swap May 14, 2023
@frederick-vs-ja frederick-vs-ja changed the title Expand std::iter_swap Expand std::iter_swap and _Swap_adl in free functions May 14, 2023
@StephanTLavavej StephanTLavavej added the throughput Must compile faster label May 15, 2023
@frederick-vs-ja frederick-vs-ja changed the title Expand std::iter_swap and _Swap_adl in free functions Expand std::iter_swap and remove _Swap_adl May 15, 2023
stl/inc/regex Outdated Show resolved Hide resolved
stl/inc/vector Outdated Show resolved Hide resolved
@StephanTLavavej
Copy link
Member

Thanks, this is a nice improvement for debuggability! I pushed a commit to remove a couple of using-declarations.

@StephanTLavavej StephanTLavavej moved this from Initial Review to Final Review in Code Reviews May 15, 2023
@StephanTLavavej
Copy link
Member

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

@strega-nil-ms strega-nil-ms moved this from Final Review to Ready To Merge in Code Reviews May 17, 2023
@CaseyCarter CaseyCarter removed their assignment May 17, 2023
@CaseyCarter CaseyCarter merged commit 71e404b into microsoft:main May 18, 2023
35 checks passed
Code Reviews automation moved this from Ready To Merge to Done May 18, 2023
@CaseyCarter
Copy link
Member

CaseyCarter commented May 18, 2023

Thanks for swapping calls to _Swap_adl with direct calls to swap!

@frederick-vs-ja frederick-vs-ja deleted the swap-adl branch May 18, 2023 23:26
JMazurkiewicz added a commit to JMazurkiewicz/STL that referenced this pull request May 30, 2023
JMazurkiewicz added a commit to JMazurkiewicz/STL that referenced this pull request Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
throughput Must compile faster
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

STL: std::iter_swap can be inlined into callers
4 participants