Skip to content

Workaround Devcom-1559808 in ranges::sort#2290

Merged
StephanTLavavej merged 5 commits into
microsoft:mainfrom
CaseyCarter:sort_bug
Nov 13, 2021
Merged

Workaround Devcom-1559808 in ranges::sort#2290
StephanTLavavej merged 5 commits into
microsoft:mainfrom
CaseyCarter:sort_bug

Conversation

@CaseyCarter
Copy link
Copy Markdown
Contributor

@CaseyCarter CaseyCarter commented Oct 21, 2021 •
edited by StephanTLavavej
Loading

If there's one thing that must work in C++, it's sorting vector =)

For ease of review: DevCom-1559808.

This also moves _Rewrap_subrange to <xutility> because it's already being used there:

STL/stl/inc/xutility

Line 5674 in ee5a216

return _Rewrap_subrange<subrange<_It1>>(_First1, _STD move(_UResult));

@CaseyCarter
Workaround Devcom-1559808 in ranges::sort
7c6d072 
If there's one thing that _must_ work in C++, it's sorting `vector` =)
@CaseyCarter CaseyCarter added the bug Something isn't working label Oct 21, 2021
@CaseyCarter CaseyCarter requested a review from a team as a code owner October 21, 2021 05:23
CaseyCarter
CaseyCarter commented Oct 21, 2021
View reviewed changes
Comment thread tests/std/tests/P0896R4_ranges_alg_sort/test.cpp Outdated
AlexGuteniev
AlexGuteniev suggested changes Oct 21, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@AlexGuteniev AlexGuteniev left a comment

Choose a reason for hiding this comment

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

Also occurs in:

STL/stl/inc/algorithm

Lines 2639 to 2643 in d8f03cf

auto [_Match, _Mid1] =
_Equal_rev_pred(_Candidate, _First2, _First2 + _Count2, _Pred, _Proj1, _Proj2);
if (_Match) {
return {_STD move(_Candidate), _STD move(_Mid1)};
}

STL/stl/inc/xutility

Lines 5715 to 5720 in d8f03cf

for (; _Count1 >= _Count2; ++_First1, (void) --_Count1) {
auto [_Match, _Mid1] = _RANGES _Equal_rev_pred(_First1, _First2, _Last2, _Pred, _Proj1, _Proj2);
if (_Match) {
return {_STD move(_First1), _STD move(_Mid1)};
}
}

STL/stl/inc/ranges

Lines 3780 to 3784 in d8f03cf

auto [_Begin, _End] = _RANGES search(subrange{_Current, _Last}, _Parent->_Pattern);
if (_Begin != _Last && _RANGES empty(_Parent->_Pattern)) {
++_Begin;
++_End;
}

STL/stl/inc/ranges

Lines 3826 to 3830 in d8f03cf

auto [_Begin, _End] = _RANGES search(subrange{_It, _Last}, _Pattern);
if (_Begin != _Last && _RANGES empty(_Pattern)) {
++_Begin;
++_End;
}

StephanTLavavej
StephanTLavavej reviewed Oct 30, 2021
View reviewed changes
Comment thread stl/inc/algorithm
Comment thread tests/std/tests/P0896R4_ranges_alg_find_end/test.cpp Outdated
Comment thread tests/std/tests/P0896R4_ranges_alg_find_end/test.cpp Outdated
Comment thread tests/std/tests/P0896R4_ranges_alg_search/test.cpp Outdated
Comment thread tests/std/tests/P0896R4_views_split/test.cpp Outdated
@StephanTLavavej
Copy link
Copy Markdown
Member

StephanTLavavej commented Oct 30, 2021

It looked like this was ready for review, so I went ahead and pushed minor changes. Please meow if you were still working on this! 🐱

@StephanTLavavej StephanTLavavej added the ranges C++20/23 ranges label Oct 30, 2021
miscco
miscco reviewed Oct 30, 2021
View reviewed changes
Comment thread tests/std/tests/P0896R4_ranges_alg_find_end/test.cpp
@CaseyCarter
Copy link
Copy Markdown
Contributor Author

CaseyCarter commented Oct 30, 2021

Reviewer note: I just inadvertently pushed a commit into this branch, and then force-pushed it back to e88474f. Hopefully this didn't reset incremental review.

@CaseyCarter CaseyCarter linked an issue Nov 8, 2021 that may be closed by this pull request
<xutility>: Use of _Rewrap_subrange which is defined in <algorithm> #2325
Closed
@StephanTLavavej
Copy link
Copy Markdown
Member

StephanTLavavej commented Nov 12, 2021

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed, or if more work is required.

@StephanTLavavej StephanTLavavej merged commit e89128e into microsoft:main Nov 13, 2021
@StephanTLavavej
Copy link
Copy Markdown
Member

StephanTLavavej commented Nov 13, 2021

Thanks for fixing these bugs! 🔧 🐞 😻

@CaseyCarter CaseyCarter deleted the sort_bug branch November 14, 2021 19:51
@mnatsuhara
Copy link
Copy Markdown
Contributor

mnatsuhara commented Nov 15, 2021

Did a post-commit review and all looks good! (Just for the record 📝)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@StephanTLavavej StephanTLavavej StephanTLavavej approved these changes

+3 more reviewers

@miscco miscco miscco approved these changes

@cpplearner cpplearner cpplearner left review comments

@AlexGuteniev AlexGuteniev AlexGuteniev requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

@StephanTLavavej StephanTLavavej

@mnatsuhara mnatsuhara

Labels

bug Something isn't working ranges C++20/23 ranges

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

<xutility>: Use of _Rewrap_subrange which is defined in <algorithm>

6 participants

@CaseyCarter @StephanTLavavej @mnatsuhara @miscco @AlexGuteniev @cpplearner