Implement P2408R5: Ranges Iterators As Inputs To Non-Ranges Algorithms #2960
Conversation
require `forward_iterator<_It>` instead of `_Is_fwd_iter_v<_It>` for parallel algorithms
also, add a `_Is_ranges_bidi_iter_v` type trait
There was a problem hiding this comment.
I'm afraid of reverting improvements introduced by #1794 ...
|
@frederick-vs-ja For this case, we should only be updating |
still need to fill out the rest of the tests
tests/std/tests/P2408R5_ranges_iterators_to_classic_algorithms/test.cpp
Outdated
Show resolved
Hide resolved
tests/std/tests/VSO_0157762_feature_test_macros/test.compile.pass.cpp
Outdated
Show resolved
Hide resolved
tests/std/tests/P2408R5_ranges_iterators_to_classic_algorithms/test.cpp
Outdated
Show resolved
Hide resolved
tests/std/tests/P2408R5_ranges_iterators_to_classic_algorithms/test.cpp
Outdated
Show resolved
Hide resolved
tests/std/tests/P2408R5_ranges_iterators_to_classic_algorithms/test.cpp
Outdated
Show resolved
Hide resolved
tests/std/tests/VSO_0157762_feature_test_macros/test.compile.pass.cpp
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
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.
|
|
||
| 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) |
There was a problem hiding this comment.
#ifdef (not worth resetting testing)
| #define __cpp_lib_concepts 202002L | ||
| #endif // !defined(__EDG__) || defined(__INTELLISENSE__) | ||
|
|
||
| #if defined(__cpp_lib_concepts) |
There was a problem hiding this comment.
Ditto #ifdef (not worth resetting testing).
|
I note that the proposal is more hardline than what we're implementing; it says "must model 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. |
|
I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed. |
|
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>
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.
Fixes #2925
Changes from paper:
forward_iterator<FwdIt>when in concepts mode for read-only iteratorsstatic_assert(_Is_blah_iter_v), turn it into astatic_assert(_Is_ranges_blah_iter_v), when the iterator is non-mutablesample: check for_Is_ranges_fwd_iter_vunique_copy: check for_Is_ranges_fwd_iter_vOther changes, not explicitly specified in the paper:
_Is_blah_iter_v, and the iterator in question is non-mutable, check_Is_ranges_blah_iter_v_Is_ranges_random_iter_vinadvanceprev: check for_Is_ranges_bidi_iter_v_Use_atomic_iterators-> check for_Is_ranges_random_iter_v- this has ABI impactDrive by:
_Can_reread_destto correctly use_Is_fwd_iter_v, not checking forforward_iterator, as it is a mutable iteratorrotate_copy(ExPo)Renamings:
_REQUIRE_PARALLEL_ITERATORbecomes two macros:_REQUIRE_PARALLEL_ITERATORchecks forforward_iteratornow, and is used for read-only iterators_REQUIRE_CPP17_MUTABLE_ITERATORchecks 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_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