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 ranges::replace #983

Merged
merged 8 commits into from
Jul 11, 2020
Merged

Conversation

miscco
Copy link
Contributor

@miscco miscco commented Jul 3, 2020

This implements the ranges::replace algorithm.

@CaseyCarter: This does not yet compile as the algorithm has some additional constraints that are not fulfilled by the generic with_input_range

The question is whether we want to specialize some with_comparable_range in the algorithm support header or simply special case it in the test. As that will most likely happen with other algorithms too I would like find a fitting solution that fits best for all algorithms.

Also the wording is really strange. IT says that the algorithm shall return last which could actually be a sentinel. I believe it is indeed meant to return respective iterator but icky wording

@miscco miscco requested a review from a team as a code owner July 3, 2020 12:14
@miscco miscco force-pushed the ranges_replace branch 5 times, most recently from e8f280f to 1840a5d Compare July 3, 2020 19:39
@StephanTLavavej StephanTLavavej added the cxx20 C++20 feature label Jul 3, 2020
Copy link
Member

@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.

This does not yet compile as the algorithm has some additional constraints that are not fulfilled by the generic with_input_range

Since this is passing now, I suspect you've realized that testing ranges with proxy references requires you to stick fairly closely to ints or pairs, or be prepared to add operations to test::proxy_reference.

also the wording is really strange. It says that the algorithm shall return last which could actually be a sentinel. I believe it is indeed meant to return respective iterator but icky wording.

See [algorithms.requirements]/13, and read [algorithms.requirements]/12 while you're there. WG21 specifies algorithms in a DSL that resembles but is not C++ ;)

stl/inc/algorithm Outdated Show resolved Hide resolved
stl/inc/algorithm Outdated Show resolved Hide resolved
stl/inc/algorithm Outdated Show resolved Hide resolved
@miscco
Copy link
Contributor Author

miscco commented Jul 5, 2020

One style question for the maintainers.

Do you prefer individual algorithm PRs like this here or should I open a single PR for e.g. all ranges::replace* algorithms?

@StephanTLavavej
Copy link
Member

I think it's generally easier to review an entire family of algorithms in a single PR, since following a consistent pattern makes reviewing similar algorithms easier - then we just need to watch out for inconsistencies and (somewhat harder) watch out for places that should be different - neither of which is helped by splitting algorithms across PRs.

Other maintainers may feel differently. It's not critically important for me.

@CaseyCarter CaseyCarter mentioned this pull request Jul 7, 2020
@miscco
Copy link
Contributor Author

miscco commented Jul 8, 2020

To ease review and reduce thenumber of concurrent PRs I have merged the other two remaining ranges::replace algorithms into this PR

@StephanTLavavej StephanTLavavej added the ranges C++20/23 ranges label Jul 8, 2020
* Extract `with_output_iterators` from `with_writable_iterators` and implement `test_in_outerator` to instantiate with `input_range` and `output_iterator` types.

* Pull `ranges::equal` out of `instantiator::call` in both `replace_copy` and `replace_copy_if` tests to avoid `/analyze` exhausting the compiler heap.
Copy link
Member

@StephanTLavavej StephanTLavavej left a comment

Choose a reason for hiding this comment

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

Will push changes to fix comment typos, otherwise looks great!

stl/inc/algorithm Show resolved Hide resolved
stl/inc/algorithm Show resolved Hide resolved
stl/inc/algorithm Outdated Show resolved Hide resolved
stl/inc/algorithm Outdated Show resolved Hide resolved
@StephanTLavavej StephanTLavavej self-assigned this Jul 10, 2020
@StephanTLavavej StephanTLavavej merged commit f357e2c into microsoft:master Jul 11, 2020
@StephanTLavavej
Copy link
Member

Thanks @miscco! You're irreplaceable. 😎

@miscco miscco deleted the ranges_replace branch July 12, 2020 08:21
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