Implement P2770R0 "Stashing stashing iterators for proper flattening" #3466

cpplearner · 2023-02-14T11:55:31Z

Fixes #3450

…range`s, pending resolution of LWG-3698 (microsoft#2727)" This reverts commit 2966813.

…erRng to be a range

stl/inc/regex

stl/inc/ranges

cpplearner · 2023-02-17T09:04:39Z

It seems that P2770 breaks iterator(iterator<!Const> i) when forward_range<V> is false and forward_range<const V> is true: said constructor initializes outer_ with std::move(i.outer_), but i.outer_ is not present in this case.

(P2770R0 correctly points out that Const can only be true when forward_range<const V> is true, so outer_ is always present.)

cc @timsong-cpp

timsong-cpp · 2023-02-17T12:57:05Z

when forward_range<V> is false and forward_range<const V> is true

That is not a scenario I care about (where's @CaseyCarter's promised paper when you need it?) - I know of no realistic range where adding const strengthens the iterator concept (it can make it not-a-range, of course, and there was some musings at one point about weakening to input for views that would otherwise need to cache).

That said, lazy_split_view does additionally constrain its const begin/end with forward_range<V>, so I'm not opposed to an LWG issue that adds that for consistency.

hewillk · 2023-02-22T04:26:11Z

I think you should define an alias to replace _InnerRng<_Const>, for example:

using _InnerBase = _InnerRng<_Const>;

since _InnerRng<_Const> is repeated too many times, it is a bit dazzling.

I believe this increases readability:

        constexpr decltype(auto) operator++(int) {
            if constexpr (_Deref_is_glvalue && forward_range<_Base> && forward_range<_InnerBase>) {
                auto _Tmp = *this;
                ++*this;
                return _Tmp;
            } else {
                ++*this;
            }
        }

StephanTLavavej · 2023-02-22T18:48:24Z

@hewillk That's pre-existing (24 occurrences) - I would have no objection to introducing such an alias, but I think it should be done in a followup PR because so much code is affected.

tests/std/tests/P2441R2_views_join_with/test.cpp

GH 3493 "<ranges>: Fix misused list-initializations" added test coverage for GH 3014 "<ranges>: list-initialization is misused". This defined an input_iterator named init_list_not_constructible_iterator. Now, P2770R0 "Stashing Stashing Iterators For Proper Flattening" is upgrading join_view and join_with_view's const overload of begin() to require forward_range<const V>. Therefore, we need to upgrade init_list_not_constructible_iterator to be a forward_iterator: * Mark its iterator_category as forward_iterator_tag. * Make it equality comparable, required for forward iterators. * Verify that it is now a forward_iterator. * Rename usage from InRange to FwdRange. Finally, this found two more improper uses of braces in <ranges>, within join_view::_Iterator_base and join_with_view::_Iterator_base.

StephanTLavavej · 2023-02-26T20:51:20Z

I've pushed a merge with main, mechanically resolving merge conflicts:
- In <ranges>, kept this PR's changes, but converted braces to parentheses in mem-initializer-lists.
- In P0896R4_views_join/test.cpp and P2441R2_views_join_with/test.cpp, resolved trivial adjacent-add conflicts.
Then, I added a commit for non-trivial semantic merge conflict resolutions. <ranges>: Fix misused list-initializations #3493 added test coverage for <ranges>: list-initialization is misused #3014. This defined an input_iterator named init_list_not_constructible_iterator. Now, this PR is upgrading join_view and join_with_view's const overload of begin() to require forward_range<const V>. Therefore, we need to upgrade init_list_not_constructible_iterator to be a forward_iterator:
- Mark its iterator_category as forward_iterator_tag.
- Make it equality comparable, required for forward iterators.
- Verify that it is now a forward_iterator.
- Rename usage from InRange to FwdRange.
Finally, this found two more improper uses of braces in <ranges> (in code added by this PR), within join_view::_Iterator_base and join_with_view::_Iterator_base.

StephanTLavavej · 2023-03-02T17:49:48Z

I'm a greedy kitty who wants treats 😼 and is speculatively mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

strega-nil-ms

Looks good! Minor change, but it's pre-existing so no need to reset testing.

strega-nil-ms · 2023-03-03T21:32:25Z

stl/inc/ranges

 #endif // ^^^ workaround ^^^

+    template <class _Ty>
+    _NODISCARD constexpr _Ty& _As_lvalue(_Ty&& _Val) noexcept {
+        return static_cast<_Ty&>(_Val);


No change requested since it's in the standard like this, but this static_cast is unnecessary I believe.

IIUC, this static_cast was unnecessary before WG21-P2266, but WG21-P2266 makes _Val an xvalue in return _Val;, and thus an explicit cast is now needed to make it not move-eligible.

strega-nil-ms · 2023-03-03T21:49:31Z

stl/inc/ranges

@@ -3567,37 +3567,36 @@ namespace ranges {

 #ifdef __clang__
    template <class _Rng> // TRANSITION, LLVM-47414
-    concept _Can_const_join = input_range<const _Rng> && is_reference_v<range_reference_t<const _Rng>>;
+    concept _Can_const_join = forward_range<const _Rng> && is_reference_v<range_reference_t<const _Rng>>


This is pre-existing, but I really don't like how _Can_const_join splits the primary template definition of _Join_view_base from the specialization; adding _As_lvalue makes this even worse. Can you move _As_lvalue and _Can_const_join above the primary template definition?

I know a guy who just created a dozen cleanup PRs - I'll tell him to add this change to one of them. 😹

StephanTLavavej · 2023-03-03T23:51:29Z

Thanks for implementing this feature, incrementing the repo towards C++23 conformance! 📈 🎉 😻

frederick-vs-ja · 2023-04-06T06:22:26Z

Will this be backported to a future version of VS2019?

It seems that #2727 hasn't been backported to VS2019 16.11.25 yet, so I'm afraid that there's already some ABI-incompatible uses of join_view between VS2019 and VS2022.

StephanTLavavej · 2023-04-06T09:42:43Z

At this time, we aren't planning to service VS 2019 for anything that isn't a security bug or a massively impactful bug. It's unfortunate that #2727 was merged days after we announced the backport (2022-05-23 vs. 2022-05-10), and while this means that ABI-incompatible uses aren't blocked, I believe that the affected code will be minimal. We completed C++20 and its DRs in VS 2019 but we can't keep changing it forever, and this change just missed the train. Users who are interested in using the most advanced features should really upgrade to VS 2022.

cpplearner added 10 commits February 14, 2023 19:00

Revert "<ranges>: Temporarily disable join_view for non-`forward_…

4ccfc5e

…range`s, pending resolution of LWG-3698 (microsoft#2727)" This reverts commit 2966813.

LWG-3698 (join_view part)

259eba9

LWG-3698 (join_with_view part)

42046d3

LWG-3698 (regex_iterator & regex_token_iterator part)

e173647

Add tests for LWG-3698

d3431f9

LWG-3700 The const begin of the join_view family does not require Inn…

02e58e5

…erRng to be a range

Add tests for LWG-3700

a701dd1

LWG-3791 join_view::iterator::operator-- may be ill-formed

3414a57

Add ProxyRef::xvalue in range_algorithm_support.hpp

fae90be

Add test for LWG-3791

aae58b7

cpplearner requested a review from a team as a code owner February 14, 2023 11:55

cpplearner added 2 commits February 14, 2023 20:55

Make join_view::iterator's constructors private

0553cc4

Simplify join_with_view::iterator::operator--

e93956e

cpplearner force-pushed the P2770R0 branch from 4f77cdf to e93956e Compare February 14, 2023 12:56

Update libc++ skips

ae4236e

StephanTLavavej added the cxx23 C++23 feature label Feb 14, 2023

frederick-vs-ja reviewed Feb 15, 2023

View reviewed changes

stl/inc/regex Outdated Show resolved Hide resolved

Add iterator_concept only if _HAS_CXX20

57cfc02

cpplearner force-pushed the P2770R0 branch from ee9c7d6 to 57cfc02 Compare February 15, 2023 10:52

Fix LWG-3791 test

9343d0c

StephanTLavavej requested changes Feb 16, 2023

View reviewed changes

stl/inc/ranges Outdated Show resolved Hide resolved

stl/inc/ranges Show resolved Hide resolved

stl/inc/ranges Outdated Show resolved Hide resolved

stl/inc/ranges Outdated Show resolved Hide resolved

stl/inc/ranges Outdated Show resolved Hide resolved

This comment was marked as resolved.

Sign in to view

cpplearner added 3 commits February 17, 2023 15:38

Address review comments from StephanTLavavej

0681646

Add tests for ranges that do not model simple-view

ba47b18

Remove redundant forward declarations of _Sentinel

f402d29

Fix test

1f68bba

StephanTLavavej reviewed Feb 22, 2023

View reviewed changes

tests/std/tests/P2441R2_views_join_with/test.cpp Outdated Show resolved Hide resolved

tests/std/tests/P2441R2_views_join_with/test.cpp Outdated Show resolved Hide resolved

Code review feedback.

63a7ffb

StephanTLavavej approved these changes Feb 22, 2023

View reviewed changes

StephanTLavavej self-assigned this Feb 24, 2023

Merge branch 'main' into P2770R0

b10ab4c

StephanTLavavej approved these changes Feb 24, 2023

View reviewed changes

This comment was marked as resolved.

Sign in to view

StephanTLavavej removed their assignment Feb 24, 2023

StephanTLavavej added 2 commits February 26, 2023 12:16

Merge branch 'main' into P2770R0

e8682cf

StephanTLavavej approved these changes Feb 26, 2023

View reviewed changes

JMazurkiewicz mentioned this pull request Mar 1, 2023

LWG-3851: chunk_view::inner-iterator missing custom iter_move and iter_swap #3517

Merged

StephanTLavavej self-assigned this Mar 2, 2023

This was referenced Mar 2, 2023

P2770R0 "Stashing stashing iterators for proper flattening" breaks test/view/tokenize.cpp ericniebler/range-v3#1771

Open

Cleanups: Friendship is magic #3524

Merged

strega-nil-ms approved these changes Mar 3, 2023

View reviewed changes

StephanTLavavej merged commit 9f751b1 into microsoft:main Mar 3, 2023

StephanTLavavej mentioned this pull request Mar 4, 2023

Various cleanups #3533

Merged

cpplearner deleted the P2770R0 branch March 4, 2023 04:03

cpplearner mentioned this pull request Dec 15, 2023

Make join_view and join_with_view iterators default-constructible (the forgotten part of P2770R0) #4264

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement P2770R0 "Stashing stashing iterators for proper flattening" #3466

Implement P2770R0 "Stashing stashing iterators for proper flattening" #3466

cpplearner commented Feb 14, 2023

This comment was marked as resolved.

cpplearner commented Feb 17, 2023

timsong-cpp commented Feb 17, 2023

hewillk commented Feb 22, 2023 •

edited

Loading

StephanTLavavej commented Feb 22, 2023

This comment was marked as resolved.

StephanTLavavej commented Feb 26, 2023 •

edited

Loading

StephanTLavavej commented Mar 2, 2023

strega-nil-ms left a comment

strega-nil-ms Mar 3, 2023

cpplearner Mar 3, 2023 •

edited

Loading

strega-nil-ms Mar 3, 2023

StephanTLavavej Mar 3, 2023

StephanTLavavej commented Mar 3, 2023

frederick-vs-ja commented Apr 6, 2023

StephanTLavavej commented Apr 6, 2023

Implement P2770R0 "Stashing stashing iterators for proper flattening" #3466

Implement P2770R0 "Stashing stashing iterators for proper flattening" #3466

Conversation

cpplearner commented Feb 14, 2023

This comment was marked as resolved.

cpplearner commented Feb 17, 2023

timsong-cpp commented Feb 17, 2023

hewillk commented Feb 22, 2023 • edited Loading

StephanTLavavej commented Feb 22, 2023

This comment was marked as resolved.

StephanTLavavej commented Feb 26, 2023 • edited Loading

StephanTLavavej commented Mar 2, 2023

strega-nil-ms left a comment

Choose a reason for hiding this comment

strega-nil-ms Mar 3, 2023

Choose a reason for hiding this comment

cpplearner Mar 3, 2023 • edited Loading

Choose a reason for hiding this comment

strega-nil-ms Mar 3, 2023

Choose a reason for hiding this comment

StephanTLavavej Mar 3, 2023

Choose a reason for hiding this comment

StephanTLavavej commented Mar 3, 2023

frederick-vs-ja commented Apr 6, 2023

StephanTLavavej commented Apr 6, 2023

hewillk commented Feb 22, 2023 •

edited

Loading

StephanTLavavej commented Feb 26, 2023 •

edited

Loading

cpplearner Mar 3, 2023 •

edited

Loading