Skip to content

Implement LWG-3591 and LWG-3592 for lazy_split_view - #2411

Merged
Stephan T. Lavavej (StephanTLavavej) merged 3 commits into
microsoft:mainfrom
cristeigabriela:main
Dec 17, 2021
Merged

Implement LWG-3591 and LWG-3592 for lazy_split_view#2411
Stephan T. Lavavej (StephanTLavavej) merged 3 commits into
microsoft:mainfrom
cristeigabriela:main

Conversation

@cristeigabriela

@cristeigabriela gabriela cristei (cristeigabriela) commented Dec 12, 2021

Copy link
Copy Markdown
Contributor

Fixes #2401 and fixes #2402.

@cristeigabriela

Copy link
Copy Markdown
Contributor Author

I notice there's issue with the tests and I will be looking into it very soon.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You will also need to adopt the tests in STL\tests\std\tests\P0896R4_views_lazy_split

Please have a look in the Readme on how to build the STL and then run those specific tests

Comment thread stl/inc/ranges Outdated
Comment thread stl/inc/ranges Outdated
@miscco

Copy link
Copy Markdown
Contributor

Here are the links to the relevant code lines for this issue:

STL/stl/inc/ranges

Lines 3673 to 3675 in 2f9f567

_NODISCARD constexpr const iterator_t<_BaseTy>& base() const& noexcept /* strengthened */ {
return _It._Get_current();
}

It would be super awesome if you could also tackle #2402 while you are in the vicinity.

The relevant lines are

STL/stl/inc/ranges

Lines 3757 to 3764 in 2f9f567

_NODISCARD constexpr auto begin() {
if constexpr (forward_range<_Vw>) {
return _Outer_iter<_Simple_view<_Vw>>{*this, _RANGES begin(_Range)};
} else {
this->_Current = _RANGES begin(_Range);
return _Outer_iter<false>{*this};
}
}

and

STL/stl/inc/ranges

Lines 3773 to 3776 in 2f9f567

_NODISCARD constexpr auto end() requires forward_range<_Vw> && common_range<_Vw> {
// clang-format on
return _Outer_iter<_Simple_view<_Vw>>{*this, _RANGES end(_Range)};
}

Note that we use _Pat for the type of the pattern so this should be && _Simple_view<_Pat>

It is totally fine if you do not want to tackle both

@cristeigabriela

Copy link
Copy Markdown
Contributor Author

Thank you very much! I'm on it (I'll be taking on what you asked me to), sorry for my lack of experience and thank you very much for being so helpful!

@miscco

Copy link
Copy Markdown
Contributor

Thank you very much! I'm on it (I'll be taking on what you asked me to), sorry for my lack of experience and thank you very much for being so helpful!

No worries, you can only get experience by trying it out and the stl is a complicated code base. If you need any help just ask right away

@cristeigabriela

Copy link
Copy Markdown
Contributor Author

A peculiarity, using the following from the official LLVM GitHub releases page:

C:\Users\gabriel\Documents\GitHub\STL>clang-format --version
clang-format version 13.0.0

Here's what clang-format says:

C:\Users\gabriel\Documents\GitHub\STL>clang-format --dry-run -Werror stl/inc/ranges
stl/inc/ranges:2464:30: error: code should be clang-formatted [-Wclang-format-violations]
            return (_STD min) (_Length, static_cast<decltype(_Length)>(_Count));
                             ^
stl/inc/ranges:2469:30: error: code should be clang-formatted [-Wclang-format-violations]
            return (_STD min) (_Length, static_cast<decltype(_Length)>(_Count));
                             ^
stl/inc/ranges:2537:51: error: code should be clang-formatted [-Wclang-format-violations]
                    _Count            = (_STD min) (_RANGES distance(_Range), _Count);
                                                  ^
stl/inc/ranges:2795:48: error: code should be clang-formatted [-Wclang-format-violations]
                const auto _Offset = (_STD min) (_RANGES distance(_Range), _Count);
                                               ^
stl/inc/ranges:2816:44: error: code should be clang-formatted [-Wclang-format-violations]
            const auto _Offset = (_STD min) (_RANGES distance(_Range), _Count);
                                           ^
stl/inc/ranges:2907:40: error: code should be clang-formatted [-Wclang-format-violations]
                    _Count = (_STD min) (_RANGES distance(_Range), _Count);
                                       ^

Am I missing something? If so, sorry.

@cristeigabriela gabriela cristei (cristeigabriela) changed the title Implement LWG-3591: `lazy_split_view<input_view>::inner-iterator::bas… Implement LWG-3591 and LWG-3592 Dec 13, 2021
@miscco

Copy link
Copy Markdown
Contributor

Thanks alot, formatting is still a pain point.

It also looks like the formatter is not too happy with some of the code but I am not sure whether it warrants turning it off.

I am slightly terrified that all Tests pass. I am going to bed now, but I hope to make some suggestions where to add Tests tomorrow if Casey Carter (@CaseyCarter) doesnt beat me to it

@fsb4000

This comment has been minimized.

@CaseyCarter

Copy link
Copy Markdown
Contributor

@cristeigabriel , Stephan T. Lavavej (@StephanTLavavej): I pushed a change to extract the boolean expression template argument out into a constexpr bool variable to make it clearer to clang-format that _Outer_iter < /* stuff */ > is a type and not an expression.

@StephanTLavavej Stephan T. Lavavej (StephanTLavavej) changed the title Implement LWG-3591 and LWG-3592 Implement LWG-3591 and LWG-3592 for lazy_split_view Dec 16, 2021
@StephanTLavavej

Copy link
Copy Markdown
Member

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

@StephanTLavavej
Stephan T. Lavavej (StephanTLavavej) merged commit 9a60c82 into microsoft:main Dec 17, 2021
@StephanTLavavej

Copy link
Copy Markdown
Member

Thanks for implementing these LWG issue resolutions and congratulations on your first microsoft/STL commit! 🎉 😻 ✅

This will ship in VS 2022 17.2 Preview 2.

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

Labels

LWG Library Working Group issue ranges C++20/23 ranges

Projects

None yet

5 participants