Implement P2408R5: Ranges Iterators As Inputs To Non-Ranges Algorithms by strega-nil-ms · Pull Request #2960 · microsoft/STL

nicole mazzuca (strega-nil-ms) · 2022-07-27T20:46:51Z

Fixes #2925

Changes from paper:

parallel algorithms: require forward_iterator<FwdIt> when in concepts mode for read-only iterators
other algorithms: if there is a static_assert(_Is_blah_iter_v), turn it into a static_assert(_Is_ranges_blah_iter_v), when the iterator is non-mutable
sample: check for _Is_ranges_fwd_iter_v
unique_copy: check for _Is_ranges_fwd_iter_v

Other changes, not explicitly specified in the paper:

many algorithms: if there is a check for different _Is_blah_iter_v, and the iterator in question is non-mutable, check _Is_ranges_blah_iter_v
- for example, check for _Is_ranges_random_iter_v in advance
prev: check for _Is_ranges_bidi_iter_v
_Use_atomic_iterators -> check for _Is_ranges_random_iter_v - this has ABI impact

Drive by:

fixes _Can_reread_dest to correctly use _Is_fwd_iter_v, not checking for forward_iterator, as it is a mutable iterator
add a missed check in rotate_copy(ExPo)

Renamings:

_REQUIRE_PARALLEL_ITERATOR becomes two macros:
- _REQUIRE_PARALLEL_ITERATOR checks for forward_iterator now, and is used for read-only iterators
- _REQUIRE_CPP17_MUTABLE_ITERATOR checks for _Is_fwd_iter_v (like the old _REQUIRE_PARALLEL_ITERATOR), but has a new message
_Is_blah_iter_v -> _Is_cpp17_blah_iter_v - makes it more obvious that we're checking for classic iterators
ABI impact from _Use_atomic_iterators
- _Static_partitioned_find2 -> _Static_partitioned_find3
- _Static_partitioned_find_end_forward -> _Static_partitioned_find_end_forward2
- _Static_partitioned_find_end_backward2 -> _Static_partitioned_find_end_backward3
- _Static_partitioned_adjacent_find2 -> _Static_partitioned_adjacent_find3
- _Static_partitioned_mismatch2 -> _Static_partitioned_mismatch3
- _Static_partitioned_search2 -> _Static_partitioned_search3
- _Static_partitioned_search_n2 -> _Static_partitioned_search_n3
- _Static_partitioned_is_sorted_until -> _Static_partitioned_is_sorted_until2
- _Static_partitioned_is_heap_until -> _Static_partitioned_is_heap_until2

require `forward_iterator<_It>` instead of `_Is_fwd_iter_v<_It>` for parallel algorithms

also, add a `_Is_ranges_bidi_iter_v` type trait

A. Jiang (frederick-vs-ja)

I'm afraid of reverting improvements introduced by #1794 ...

nicole mazzuca (strega-nil-ms) · 2022-07-28T18:16:35Z

A. Jiang (@frederick-vs-ja) For this case, we should only be updating static_asserts and the specific algorithms specified in the paper, and therefore should not have made any changes that break performance.

still need to fill out the rest of the tests

Stephan T. Lavavej (StephanTLavavej) · 2022-08-02T01:18:12Z

Thanks, this looks good. I've pushed changes for extremely minor test nitpicks, and a tiny bit of missing coverage in the feature-test macro test (also fixing a pre-existing occurrence).

Casey Carter (CaseyCarter)

General comment: I think the wording is strong enough to give us the power to test for sized_sentinel and use it to compute distances for sub-random_access iterators. The potential for breakage is high and the potential benefit smallish, so I'd be happy to see a followup issue to investigate rather than more changes in this PR.

Casey Carter (CaseyCarter) · 2022-08-02T23:10:38Z

 template <class _Iter>
-_INLINE_VAR constexpr bool _Is_random_iter_v = is_convertible_v<_Iter_cat_t<_Iter>, random_access_iterator_tag>;
+_INLINE_VAR constexpr bool _Is_ranges_random_iter_v =
+#if defined(__cpp_lib_concepts)


#ifdef (not worth resetting testing)

Casey Carter (CaseyCarter) · 2022-08-02T23:56:19Z

+#define __cpp_lib_concepts 202002L
+#endif // !defined(__EDG__) || defined(__INTELLISENSE__)
+
+#if defined(__cpp_lib_concepts)


Ditto #ifdef (not worth resetting testing).

Casey Carter (CaseyCarter) · 2022-08-03T01:43:50Z

I note that the proposal is more hardline than what we're implementing; it says "must model meow_iterator" where we're enforcing more "must model meow_iterator or meet the Cpp17MeowIterator requirements". I think I'm fine with this as-is; the requirements are all preconditions rather than constraints, so we're welcome to define behavior for violations.

EDIT: I forgot to point out that this relaxation of the strict requirements of the proposal is a large part of why I'm comfortable making the change in C++20 mode.

Stephan T. Lavavej (StephanTLavavej) · 2022-08-03T03:08:43Z

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

Stephan T. Lavavej (StephanTLavavej) · 2022-08-03T23:07:31Z

Thanks for implementing this C++23 feature/overhaul! 😻 🚀 ✅

microsoft#2960) Co-authored-by: Nicole Mazzuca <mazzucan@outlook.com> Co-authored-by: Stephan T. Lavavej <stl@nuwen.net>

The overloads of for_each that take an execution policy argument do not require that the iterator arguments always be mutable iterators. Whether or not the iterators need to be mutable depends on what the function object does. for_each is unusual, if not unique, in this regard; for all other algorithms the mutability of the iterators is specified by the algorithm. See http://eel.is/c++draft/alg.foreach#7 , though I admit that the wording is not particularly clear about this. Since the compiler can't realiably figure out whether or not the function object modifies the elements in the sequence, for_each should only check for constant iterators (_REQUIRE_PARALLEL_ITERATOR), not mutable iterators (_REQUIRE_CPP17_MUTABLE_ITERATOR). This bug was introduced by microsoft#2960 . That PR should not have changed for_each.

nicole mazzuca (strega-nil-ms) requested a review from a team as a code owner July 27, 2022 20:46

nicole mazzuca (strega-nil-ms) marked this pull request as draft July 27, 2022 21:31

nicole mazzuca (strega-nil-ms) force-pushed the p2408 branch from f7d037e to 9285f7e Compare July 27, 2022 22:32

nicole mazzuca (strega-nil) added 2 commits July 27, 2022 15:34

P2408R5: parallel algorithms

0a3430a

require `forward_iterator<_It>` instead of `_Is_fwd_iter_v<_It>` for parallel algorithms

do the other changes

2103356

also, add a `_Is_ranges_bidi_iter_v` type trait

nicole mazzuca (strega-nil-ms) force-pushed the p2408 branch from 9285f7e to 2103356 Compare July 27, 2022 23:11

nicole mazzuca (strega-nil-ms) marked this pull request as ready for review July 27, 2022 23:11

Casey Carter (CaseyCarter) added the cxx23 C++23 feature label Jul 28, 2022

A. Jiang (frederick-vs-ja) reviewed Jul 28, 2022

View reviewed changes

Comment thread stl/inc/xutility Outdated

Comment thread stl/inc/xutility Outdated

frederick-vs-ja's comments

96c60cc

nicole mazzuca (strega-nil-ms) commented Jul 28, 2022

View reviewed changes

Comment thread stl/inc/xutility

_Can_reread_dest can be a concept

f6d471f

Stephan T. Lavavej (StephanTLavavej) requested changes Jul 29, 2022

View reviewed changes

Michael Schellenberger Costa (miscco) reviewed Jul 29, 2022

View reviewed changes

Comment thread stl/inc/xutility Outdated

Comment thread stl/inc/xutility Outdated

Comment thread stl/inc/algorithm

Comment thread stl/inc/functional

nicole mazzuca (strega-nil) added 7 commits July 29, 2022 09:19

CR comments I agree with

4354bcd

add test framework

aed6afc

still need to fill out the rest of the tests

forgot the test.lst

158d873

wow that was a dumb bug

dd099a2

more dumb bug

37898ed

add tests

8293ef2

remove updates to vector

58d98f9

A. Jiang (frederick-vs-ja) reviewed Jul 30, 2022

View reviewed changes

Comment thread tests/std/tests/P2408R5_ranges_iterators_to_classic_algorithms/test.cpp Outdated

nicole mazzuca (strega-nil) and others added 3 commits August 1, 2022 16:29

fix tests

59e775f

Test nitpicks: Include headers, drop std::.

a4391db

Detect macros being defined when they shouldn't.

8283a09

Stephan T. Lavavej (StephanTLavavej) reviewed Aug 2, 2022

View reviewed changes

Stephan T. Lavavej (StephanTLavavej) approved these changes Aug 2, 2022

View reviewed changes

Casey Carter (CaseyCarter) reviewed Aug 2, 2022

View reviewed changes

Stephan T. Lavavej (StephanTLavavej) assigned nicole mazzuca (strega-nil-ms) Aug 2, 2022

nicole mazzuca (strega-nil) added 3 commits August 2, 2022 11:37

fix non-behavior Casey CRs

aea8deb

I think this should assuage Casey's comments

0b38f67

remove extra REQUIREs

eccf5b4

Stephan T. Lavavej (StephanTLavavej) requested changes Aug 2, 2022

View reviewed changes

Comment thread stl/inc/execution

Comment thread stl/inc/xutility Outdated

Comment thread stl/inc/execution Outdated

missed some requires

687e8c6

Stephan T. Lavavej (StephanTLavavej) unassigned nicole mazzuca (strega-nil-ms) Aug 3, 2022

Casey Carter (CaseyCarter) self-requested a review August 3, 2022 01:30

Casey Carter (CaseyCarter) approved these changes Aug 3, 2022

View reviewed changes

Stephan T. Lavavej (StephanTLavavej) approved these changes Aug 3, 2022

View reviewed changes

Stephan T. Lavavej (StephanTLavavej) mentioned this pull request Aug 3, 2022

<algorithm>, <execution>, <xutility>: Parallel algorithms inconsistently _REQUIRE_MEOW #2985

Closed

Stephan T. Lavavej (StephanTLavavej) self-assigned this Aug 3, 2022

Stephan T. Lavavej (StephanTLavavej) merged commit d9b4534 into microsoft:main Aug 3, 2022

Stephan T. Lavavej (StephanTLavavej) mentioned this pull request Aug 8, 2022

P1206R7 Conversions From Ranges To Containers #2806

Merged

David Olsen (dkolsen-pgi) mentioned this pull request Aug 25, 2022

Parallel for_each iterators don't have to be mutable #3056

Merged

nicole mazzuca (strega-nil-ms) deleted the p2408 branch December 19, 2022 23:42

Uh oh!

Conversation

nicole mazzuca (strega-nil-ms) commented Jul 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

A. Jiang (frederick-vs-ja) left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nicole mazzuca (strega-nil-ms) commented Jul 28, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Stephan T. Lavavej (StephanTLavavej) commented Aug 2, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Casey Carter (CaseyCarter) left a comment

Choose a reason for hiding this comment

Uh oh!

Casey Carter (CaseyCarter) Aug 2, 2022

Choose a reason for hiding this comment

Uh oh!

Casey Carter (CaseyCarter) Aug 2, 2022

Choose a reason for hiding this comment

Uh oh!

Casey Carter (CaseyCarter) commented Aug 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Stephan T. Lavavej (StephanTLavavej) commented Aug 3, 2022

Uh oh!

Stephan T. Lavavej (StephanTLavavej) commented Aug 3, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

nicole mazzuca (strega-nil-ms) commented Jul 27, 2022 •

edited

Loading

A. Jiang (frederick-vs-ja) left a comment •

edited

Loading

Casey Carter (CaseyCarter) commented Aug 3, 2022 •

edited

Loading