Skip to content

Conversation

@miscco
Copy link
Contributor

@miscco miscco commented Oct 15, 2020

Partially addresses #39

Note: Reading the requirement in http://eel.is/c++draft/ranges#range.counted-2 I was lazy and assumed that it is sufficient that the passed argument is convertible. Is that correct or do we need to add some _Choice magic?

@miscco miscco requested a review from a team as a code owner October 15, 2020 11:04
@miscco
Copy link
Contributor Author

miscco commented Oct 15, 2020

@CaseyCarter, I am unsure whether we should test anything more as we essentially tested everything already with the span and subrange tests.

Other test ideas besides checking for content?

@StephanTLavavej StephanTLavavej added cxx20 C++20 feature ranges C++20/23 ranges labels Oct 16, 2020
@miscco
Copy link
Contributor Author

miscco commented Oct 16, 2020

Urgh, that conversion warning of auto result = ranges::views::counted(Iter{input}, 3); sets me off.

I guess I should have been more careful regarding the convertible_to<iter_difference<_Iter>> requirement

@miscco
Copy link
Contributor Author

miscco commented Oct 16, 2020

@CaseyCarter I feel like the convertible_to<_Diff, iter_difference_t<_It>> requirement on views::counted would also make sense for counted_iterator, where we explicitly require the second argument to be of type iter_difference_t<_It>

This was also the reason that in the test we have these two lines in the test:

            counted_iterator<Iter> constructed(Iter{input}, iter_difference_t<Iter>{2});
            counted_iterator<Iter> constructedEmpty{Iter{input}, iter_difference_t<Iter>{0}};

Would you say that this is something we might want to change?

@miscco
Copy link
Contributor Author

miscco commented Oct 16, 2020

Also the actual error is that std::span takes an size_t as second argument, so I assume we would need to cast it to size_t before passing it so std::span even if that might be impossible :(

@CaseyCarter CaseyCarter self-assigned this Oct 21, 2020
Copy link
Contributor

@CaseyCarter CaseyCarter left a comment

Choose a reason for hiding this comment

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

Not a real review, just correcting the problem from #1393.

* Missed a `decay_t` in `operator()`

* Discard the result in the death test

* Only death test when `_DEBUG` (since the check is an `_STL_ASSERT`)
I'm not paranoid!
@CaseyCarter CaseyCarter removed their assignment Oct 30, 2020
@CaseyCarter CaseyCarter self-assigned this Nov 3, 2020
... resolving simple conflicts from adding things in the same place in both `tests/std/test.lst` and `tests/std/tests/P0896R4_ranges_range_machinery`.
@CaseyCarter CaseyCarter merged commit 2380824 into microsoft:master Nov 4, 2020
@CaseyCarter
Copy link
Contributor

CaseyCarter commented Nov 4, 2020

Every contribution counts! (But some more than others.)

@miscco miscco deleted the views_counted branch November 7, 2020 20:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cxx20 C++20 feature ranges C++20/23 ranges

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants