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

Implement P2474R2 views::repeat #3142

Merged
merged 39 commits into from
Jan 28, 2023
Merged

Conversation

SuperWig
Copy link
Contributor

@SuperWig SuperWig commented Oct 4, 2022

Fixes #2933

Some questions:

  • Is the custom array list visualiser worth the novelty?
    • is it possible to use the value in the item name? My first thought for the iterators was to have it like [{_Current)] {Value}
  • What would be the right noexcept spec for the reconstruction stuff? I just left one as true
    • For that matter I'm not sure what the sensible way to test the noexceptness of the tuple overload.
  • Better name or methods for the noexcept testing helpers would be nice.
  • Any interesting views that is missing coverage?

Most of the test is reworked from the iota_view test, I might have removed something that should have been kept/reworked.

Drive-by fixed single_views visualizer, I'm assuming the intent was to wrap it in []

@SuperWig SuperWig requested a review from a team as a code owner October 4, 2022 14:16
@CaseyCarter CaseyCarter added the cxx23 C++23 feature label Oct 4, 2022
@CaseyCarter CaseyCarter added the ranges C++20/23 ranges label Oct 4, 2022
@strega-nil-ms strega-nil-ms self-assigned this Oct 4, 2022
Copy link
Contributor

@strega-nil-ms strega-nil-ms left a comment

Choose a reason for hiding this comment

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

Thanks so much! Just some minor changes requested

thumbs up!

stl/debugger/STL.natvis Outdated Show resolved Hide resolved
stl/debugger/STL.natvis 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
stl/inc/ranges Outdated Show resolved Hide resolved
stl/inc/ranges Outdated Show resolved Hide resolved
stl/inc/ranges Outdated Show resolved Hide resolved
tests/std/tests/P2474R2_views_repeat/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/P2474R2_views_repeat/test.cpp Outdated Show resolved Hide resolved
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.

Several functions should be unconditionally marked with noexcept, since operations on integer-like types are all noexcept, and the iterator is implemented as a trivially copyable type.

stl/inc/ranges Outdated 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
stl/inc/ranges Outdated 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
stl/inc/ranges Outdated Show resolved Hide resolved
stl/inc/ranges Outdated Show resolved Hide resolved
@SuperWig
Copy link
Contributor Author

SuperWig commented Oct 12, 2022

Just to be clear so I know where to pick up tomorrow when you lot (except a certain vampire) are probably asleep. The noexcept-ness shouldn't depend on the bound type at all?

Edit: oh, didn't see the "11 comments" button 😅.

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.

One internal constructor can't be unconditionally noexcept.

The noexcept-ness shouldn't depend on the bound type at all?

I think it does depend. But the permitted set of bound types is very small (only integer-like types and unreachable_sentinel_t), and noexcept-ness is always hold for its elements.

stl/inc/ranges Outdated Show resolved Hide resolved
stl/inc/ranges Outdated Show resolved Hide resolved
@strega-nil-ms strega-nil-ms removed their assignment Oct 14, 2022
@StephanTLavavej

This comment was marked as resolved.

@StephanTLavavej StephanTLavavej removed their assignment Dec 7, 2022
stl/inc/utility 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
tests/std/tests/P2474R2_views_repeat/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/P2474R2_views_repeat/test.cpp Show resolved Hide resolved
tests/std/tests/P2474R2_views_repeat/test.cpp Show resolved Hide resolved
tests/std/tests/P2474R2_views_repeat/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/P2474R2_views_repeat/test.cpp Outdated Show resolved Hide resolved
@StephanTLavavej

This comment was marked as resolved.

@StephanTLavavej StephanTLavavej self-assigned this Jan 25, 2023
stl/inc/ranges Outdated Show resolved Hide resolved
stl/inc/ranges Outdated Show resolved Hide resolved
stl/inc/ranges Outdated Show resolved Hide resolved
tests/std/tests/P2474R2_views_repeat/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/P2474R2_views_repeat/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/P2474R2_views_repeat_death/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/P2474R2_views_repeat_death/test.cpp Outdated Show resolved Hide resolved
@StephanTLavavej StephanTLavavej removed their assignment Jan 26, 2023
@StephanTLavavej
Copy link
Member

Thanks! I pushed some minor changes (most notable being a slight expansion of death test coverage) and I believe that all outstanding comments have been resolved. Onwards to final review!

@CaseyCarter CaseyCarter self-requested a review January 27, 2023 00:37
@StephanTLavavej StephanTLavavej self-assigned this Jan 27, 2023
@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 1868694 into microsoft:main Jan 28, 2023
@StephanTLavavej
Copy link
Member

Thanks! Thanks! Thanks! Thanks! Thanks! Thanks! Thanks! Thanks! Thanks! Thanks! 😹 😹 😹

@CaseyCarter
Copy link
Member

You can say that again.

@SuperWig SuperWig deleted the repeat branch January 28, 2023 08:17
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
None yet
Development

Successfully merging this pull request may close these issues.

P2474R2 views::repeat
8 participants