Implement ranges::elements_view #1406

miscco · 2020-10-27T12:00:26Z

This is not fully functional as I has hit a roadblock, trying to understand why ranges::view<EV> breaks due to ranges::end not being available.

@CaseyCarter: any ideas what is going on here?

stl/inc/ranges

Co-authored-by: S. B. Tam <cpplearner@outlook.com>

stl/inc/ranges

cpplearner · 2020-10-27T14:20:52Z

Note that elements_view as specified in the current WD is broken. See LWG-3406.

miscco · 2020-10-27T15:09:38Z

Note that elements_view as specified in the current WD is broken. See LWG-3406.

Awesome, thanks for the heads up 😺

stl/inc/ranges

tests/std/tests/P0896R4_views_elements/test.cpp

stl/inc/ranges

Co-authored-by: Casey Carter <cartec69@gmail.com> Co-authored-by: S. B. Tam <cpplearner@outlook.com>

CaseyCarter

Changes look good - I think we're only missing some conditional noexcept test coverage now (#1406 (comment)).

product code: * Define `iterator_concept` so the view can adapt ranges whose iterators have no `iterator_category` (This is how we've dealt with LWG-3289 elsewhere.) * Don't try to modify members of constant iterators * `_Sentinel`'s friends are not friends of `_Iterator` test: * Silence shadowing warnings by using the global `expected_keys` and `expected_values` instead of passing them to `test_one` via parameters with the same names. * Replace references to `EV` with `R` since they name the same type * Don't `forward` the same rvalue input range repeatedly

CaseyCarter

Changes look good - I think we're only missing some conditional noexcept test coverage now (#1406 (comment)).

Nice try, but you were wrong. Fixed up a few small issues and I think this is now good to go.

StephanTLavavej

(This is a very incomplete review; I'll resume reviewing later this week)

stl/inc/ranges

StephanTLavavej

This looks great, thanks! All I found were extremely minor nitpicks so I'll validate and push changes.

stl/inc/ranges

tests/std/test.lst

tests/std/tests/P0896R4_views_elements/test.cpp

StephanTLavavej

@CaseyCarter I pushed small changes after you approved.

... and resolve `common_view` vs. `elements_view` conflict by relocating `common_view` before `reverse_view` where it belongs.

CaseyCarter · 2020-11-04T02:29:12Z

@CaseyCarter I pushed small changes after you approved.

@StephanTLavavej I pushed what look like bug changes after you approved: common_view and elements_view conflicted by being at the same point in <ranges>, which I resolved by relocating common_view before reverse_view in proper synopsis order. There are no other changes in that commit, it's just a cut-n-paste job.

CaseyCarter · 2020-11-04T02:30:00Z

@StephanTLavavej I pushed what look like bug changes after you approved: common_view and elements_view conflicted by

Err, "big changes". This is too funny to edit in place, I'm leaving the typo for posterity ;)

StephanTLavavej · 2020-11-06T12:09:29Z

FYI @CaseyCarter, I've pushed a fix after you approved, working around an internal compiler assertion in the test that I've reduced and reported.

StephanTLavavej · 2020-11-06T23:55:47Z

git automatically merged <ranges>; Implement drop_while_view #1366 added drop_while_view next to where this PR moved common_view. I double-checked that they're in that order (synopsis order).
Fixed trivial merge conflicts:
- In tests/std/test.lst, Implement drop_while_view #1366 added P0896R4_views_drop_while[_death] and this PR added P0896R4_views_elements. Accepted both, in that order (sorted order).
- In P0896R4_ranges_range_machinery/test.cpp, Implement drop_while_view #1366 added a line testing drop_while and this PR added a line testing elements<42>. Accepted both, in that order (sorted order).

StephanTLavavej · 2020-11-07T00:09:51Z

Thanks for implementing these crucial elements of ranges; customers will be able to view them in VS 2019 16.9 Preview 2. 😹

Implement ranges::elements_view

2326dc4

miscco requested a review from a team as a code owner October 27, 2020 12:00

cpplearner reviewed Oct 27, 2020

View reviewed changes

stl/inc/ranges Outdated Show resolved Hide resolved

Fix ADL issue

a6e22be

Co-authored-by: S. B. Tam <cpplearner@outlook.com>

miscco commented Oct 27, 2020

View reviewed changes

stl/inc/ranges Show resolved Hide resolved

Adopt resolution to LWG-3406

7fc23a4

miscco force-pushed the elements_view branch from dc93f24 to 7fc23a4 Compare October 27, 2020 15:34

StephanTLavavej added cxx20 C++20 feature ranges C++20/23 ranges labels Oct 27, 2020

CaseyCarter self-assigned this Oct 27, 2020

CaseyCarter suggested changes Oct 28, 2020

View reviewed changes

cpplearner reviewed Oct 28, 2020

View reviewed changes

stl/inc/ranges Outdated Show resolved Hide resolved

miscco and others added 2 commits October 28, 2020 07:47

Apply suggestions from code review

f63a85e

Co-authored-by: Casey Carter <cartec69@gmail.com> Co-authored-by: S. B. Tam <cpplearner@outlook.com>

More review comments from Casey

9fd8550

miscco force-pushed the elements_view branch from 7148ca2 to 9fd8550 Compare October 28, 2020 07:32

CaseyCarter suggested changes Oct 28, 2020

View reviewed changes

miscco mentioned this pull request Oct 28, 2020

Improve test coverage of ranges::views #1409

Open

CaseyCarter approved these changes Oct 30, 2020

View reviewed changes

CaseyCarter removed their assignment Oct 30, 2020

miscco and others added 3 commits October 30, 2020 22:19

Also verify views::keys and views::values

4ec4a61

views::{keys, values} are CPOs, not types

cbaff74

Add LWG issue numbers

d6bed47

StephanTLavavej mentioned this pull request Nov 3, 2020

Fix noexcept(noexcept(is_nothrow_v)) #1417

Merged

StephanTLavavej reviewed Nov 3, 2020

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

StephanTLavavej self-assigned this Nov 3, 2020

miscco and others added 2 commits November 3, 2020 19:57

Fix bug with noexcept specification

d5ad627

Rename _Iterator_base to _Category_base

dc12097

StephanTLavavej reviewed Nov 4, 2020

View reviewed changes

stl/inc/ranges Outdated Show resolved Hide resolved

tests/std/test.lst Show resolved Hide resolved

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

StephanTLavavej added 2 commits November 3, 2020 17:10

Merge branch 'master' into gh1406_elements_view

87da6fa

Code review feedback.

9f566e6

StephanTLavavej approved these changes Nov 4, 2020

View reviewed changes

StephanTLavavej removed their assignment Nov 4, 2020

CaseyCarter approved these changes Nov 4, 2020

View reviewed changes

Merge remote-tracking branch 'origin/master' into elements_view

daeb719

... and resolve `common_view` vs. `elements_view` conflict by relocating `common_view` before `reverse_view` where it belongs.

StephanTLavavej approved these changes Nov 4, 2020

View reviewed changes

StephanTLavavej added a commit to StephanTLavavej/STL that referenced this pull request Nov 5, 2020

microsoftGH-1406 Implement ranges::elements_view

f39eaf7

StephanTLavavej self-assigned this Nov 5, 2020

StephanTLavavej added a commit to StephanTLavavej/STL that referenced this pull request Nov 5, 2020

microsoftGH-1406 Implement ranges::elements_view

f0a9aa8

Work around VSO-1243263, an internal compiler assertion.

fcf52b5

CaseyCarter approved these changes Nov 6, 2020

View reviewed changes

Merge branch 'master' into gh1406

0da8675

StephanTLavavej merged commit 7fd6501 into microsoft:master Nov 7, 2020

StephanTLavavej mentioned this pull request Nov 7, 2020

P0896R4 <ranges> #39

Closed

miscco deleted the elements_view branch November 7, 2020 20:45

Implement ranges::elements_view #1406

Implement ranges::elements_view #1406

Conversation

miscco commented Oct 27, 2020

Uh oh!

Uh oh!

Uh oh!

cpplearner commented Oct 27, 2020

Uh oh!

miscco commented Oct 27, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

CaseyCarter left a comment

Choose a reason for hiding this comment

Uh oh!

CaseyCarter left a comment

Choose a reason for hiding this comment

Uh oh!

StephanTLavavej left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

StephanTLavavej left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

StephanTLavavej left a comment

Choose a reason for hiding this comment

Uh oh!

CaseyCarter commented Nov 4, 2020

Uh oh!

CaseyCarter commented Nov 4, 2020

Uh oh!

StephanTLavavej commented Nov 6, 2020

Uh oh!

StephanTLavavej commented Nov 6, 2020

Uh oh!

StephanTLavavej commented Nov 7, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants