Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

P2278R4: cbegin should always return a constant iterator ("Iterators" section only) #3043

Merged
merged 28 commits into from
Oct 24, 2022

Conversation

JMazurkiewicz
Copy link
Contributor

@JMazurkiewicz JMazurkiewicz commented Aug 19, 2022

Implements "Iterators" section from P2278R4 - cbegin should always return a constant iterator. Towards #2918.

@JMazurkiewicz JMazurkiewicz changed the title P2278R4: std::const_iterator P2278R4: cbegin should always return a constant iterator Aug 19, 2022
@JMazurkiewicz JMazurkiewicz changed the title P2278R4: cbegin should always return a constant iterator P2278R4: cbegin should always return a constant iterator ("Iterators" section only) Aug 19, 2022
@CaseyCarter CaseyCarter added ranges C++20/23 ranges cxx23 C++23 feature labels Aug 20, 2022
@StephanTLavavej StephanTLavavej added this to Work In Progress in Code Reviews Aug 20, 2022
@hewillk
Copy link
Contributor

hewillk commented Sep 5, 2022

Just curious, does the current implementation pass the following two assertions?

using CI = std::basic_const_iterator<int*>;
static_assert(std::sentinel_for<CI, int*>);
static_assert(std::sized_sentinel_for<CI, CI>);

@JMazurkiewicz
Copy link
Contributor Author

Just curious, does the current implementation pass the following two assertions?

Nope, it doesn't.

There was short discussion on discord about it: https://discord.com/channels/737189251069771789/738835809238646854/1011456843106762892.

@CaseyCarter
Copy link
Member

Heads up: I pushed some commits that (1) implement the proposed resolution of LWG-3769, which is intended to correct the constraint recursion in the design, and (2) workaround LLVM-55945 to avoid constraint recursion in practice.

@CaseyCarter
Copy link
Member

@JMazurkiewicz is this PR ready to become non-draft now?

@JMazurkiewicz
Copy link
Contributor Author

@CaseyCarter yes, it is. And thank you for implementing LWG-3769!

@JMazurkiewicz JMazurkiewicz marked this pull request as ready for review September 16, 2022 09:57
@JMazurkiewicz JMazurkiewicz requested a review from a team as a code owner September 16, 2022 09:57
@CaseyCarter CaseyCarter moved this from Work In Progress to Final Review in Code Reviews Sep 20, 2022
Copy link
Contributor

@frederick-vs-ja frederick-vs-ja left a comment

Choose a reason for hiding this comment

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

LWG-3765 has been set to Tentatively Ready, so I think it's safe to implement the resolution without comments.

Feel free to do this in a separated PR. I'm not sure whether the constraints should be tested.

stl/inc/xutility Outdated Show resolved Hide resolved
stl/inc/xutility Outdated Show resolved Hide resolved
@StephanTLavavej

This comment was marked as resolved.

stl/inc/xutility Outdated Show resolved Hide resolved
stl/inc/xutility Outdated Show resolved Hide resolved
tests/std/include/range_algorithm_support.hpp Show resolved Hide resolved
@@ -1404,6 +1404,14 @@ SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
</Type>


<Type Name="std::basic_const_iterator&lt;*&gt;">
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for adding a visualizer! 😸

Mirroring note: As usual, this will need to be triple-mirrored to the VS repo, but I'm ok with deferring that until the next time we have a decent-sized batch of visualizer updates.

tests/std/tests/P2278R4_basic_const_iterator/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/P2278R4_basic_const_iterator/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/P2278R4_basic_const_iterator/test.cpp Outdated Show resolved Hide resolved
@StephanTLavavej
Copy link
Member

StephanTLavavej commented Oct 20, 2022

Thanks for implementing this part of the feature! 😻 FYI @strega-nil-ms, I pushed changes after you approved.

I have a major outstanding question about HasPeek in the test, where I'm 80% sure we're silently missing test coverage, and a minor question about the comment change in range_algorithm_support.hpp. No remaining concerns with the product code.

@StephanTLavavej StephanTLavavej moved this from Final Review to Ready To Merge in Code Reviews Oct 21, 2022
@StephanTLavavej StephanTLavavej self-assigned this Oct 21, 2022
@StephanTLavavej
Copy link
Member

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

@StephanTLavavej StephanTLavavej merged commit bb0938b into microsoft:main Oct 24, 2022
Code Reviews automation moved this from Ready To Merge to Done Oct 24, 2022
@StephanTLavavej
Copy link
Member

Thanks for constantly making the STL better! 😹 😻 😸

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cxx23 C++23 feature ranges C++20/23 ranges
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

7 participants