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.

Sign up for GitHub

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

Jump to bottom

Implement ranges::copy_backward #1026

Merged
merged 5 commits into from
Jul 17, 2020
Merged

Implement ranges::copy_backward #1026

merged 5 commits into from
Jul 17, 2020

Conversation

CaseyCarter
Copy link
Member

@CaseyCarter CaseyCarter commented Jul 9, 2020 •
edited
Loading

With many thanks to @ahanamuk who did much of the work.

Partially addresses #39.

@CaseyCarter CaseyCarter added cxx20 C++20 feature ranges C++20/23 ranges labels Jul 9, 2020
@CaseyCarter CaseyCarter requested a review from a team as a code owner July 9, 2020 16:57
@CaseyCarter CaseyCarter mentioned this pull request Jul 9, 2020
Closed
@CaseyCarter CaseyCarter marked this pull request as draft July 9, 2020 18:01
miscco
miscco reviewed Jul 9, 2020
View reviewed changes
Copy link
Contributor

@miscco miscco left a comment

Choose a reason for hiding this comment

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

So this seems correct modulo some few nitpicks.

What I do not like ( and I mean like in the sense that I do not have any technical objection) is that it conflates 3 different things.

  1. It implements the algortihms copy_backward
  2. It simplifies some test machinery
  3. It introduces the TMP optimizations for ranges algorithms.

That not only makes this PR way more complex that the actual algorithm but also introduces a lot of inconsistencies that would have been better addressed with separate PRs.

I wont complain that copy_backward is fast but I will complain that it is different and the others are slow

tests/std/include/range_algorithm_support.hpp Show resolved Hide resolved
tests/std/tests/P0896R4_ranges_alg_copy_backward/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/P0896R4_ranges_alg_copy_backward/test.cpp Show resolved Hide resolved
tests/std/tests/P0896R4_ranges_alg_copy_backward/test.cpp Outdated Show resolved Hide resolved
tests/std/include/range_algorithm_support.hpp Outdated Show resolved Hide resolved
tests/std/tests/P0896R4_ranges_alg_copy_backward/test.cpp Show resolved Hide resolved
stl/inc/algorithm Show resolved Hide resolved
@CaseyCarter CaseyCarter marked this pull request as ready for review July 10, 2020 04:43
@StephanTLavavej
Copy link
Member

I'll second @miscco's concern that this fuses together changes which could apparently be separate. If they could be disentangled, it would be easier to review (and serve as a good example for contributors). If that's more work than it's worth, then it's okay in the name of expediency.

StephanTLavavej
StephanTLavavej requested changes Jul 10, 2020
View reviewed changes
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.

Having reviewed the whole PR, I'm ok with its combined state (i.e. not worth the time expenditure to decompose). Looks good, minor concerns. Thanks!

stl/inc/algorithm Outdated Show resolved Hide resolved
stl/inc/yvals_core.h Outdated Show resolved Hide resolved
tests/std/include/range_algorithm_support.hpp Outdated Show resolved Hide resolved
tests/std/tests/P0896R4_ranges_alg_copy_backward/test.cpp Show resolved Hide resolved
tests/std/tests/P0896R4_ranges_alg_copy_backward/test.cpp Outdated Show resolved Hide resolved
@CaseyCarter
Copy link
Member Author

What I do not like ( and I mean like in the sense that I do not have any technical objection) is that it conflates 3 different things.

  1. It implements the algortihms copy_backward
  2. It simplifies some test machinery
  3. It introduces the TMP optimizations for ranges algorithms.

That not only makes this PR way more complex that the actual algorithm but also introduces a lot of inconsistencies that would have been better addressed with separate PRs.

This is a righteous comment: I let this one get away from me. I'll try to submit more quantized PRs in the future. (And by "in the future" I mean "after reverse ;))

StephanTLavavej
StephanTLavavej requested changes Jul 11, 2020
View reviewed changes
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.

I can push a formatting change.

tests/std/include/range_algorithm_support.hpp Outdated Show resolved Hide resolved
@StephanTLavavej

This comment has been minimized.

Sign in to view
@StephanTLavavej

This comment has been minimized.

Sign in to view
@CaseyCarter
Implement ranges::copy_backward
8ba3445 
Workaround new occurrences of VSO-938163 triggered in the heap algorithm tests by my unwrapping-to-pointers change.

Wrap lines to 120 columns.
@CaseyCarter
Wrap more lines to 120 columns.
47bf430 
Workaround VSO-938163 in triggered in `P0896R4_ranges_alg_move` as well.

Remove `/analyze` "help" from ranges tests.

Re-enable `/analyze:only` coverage in `concepts_matrix.lst`.

Apply improved workaround for microsoftGH-1030: don't test `constexpr` under `/analyze`.

Add missing return type check in `P0896R4_ranges_alg_move`.
@CaseyCarter
Copy link
Member Author

**** NOTE TO REVIEWERS ***

I squashed-and-rebased what was here onto master to semi-reset, and added a new commit with additional changes. You should be ably to quickly scan the first commit to verify that it contains everything up though Stephan's merge conflict fix, and then focus on the changes in the second commit. That second commit primarily removes all earlier attempts to avoid triggering GH-1030 and applies a new localized workaround to avoid testing constexpr when /analyze is enabled.

@mnatsuhara mnatsuhara self-assigned this Jul 15, 2020
mnatsuhara
mnatsuhara approved these changes Jul 15, 2020
View reviewed changes
Copy link
Contributor

@mnatsuhara mnatsuhara left a comment

Choose a reason for hiding this comment

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

This comment seems even more nitpicky when I have no other comments to make.

tests/std/tests/P0896R4_ranges_test_machinery/test.cpp Show resolved Hide resolved
@CaseyCarter CaseyCarter self-assigned this Jul 16, 2020
@CaseyCarter CaseyCarter merged commit 21acd23 into microsoft:master Jul 17, 2020
@CaseyCarter
Copy link
Member Author

CaseyCarter commented Jul 17, 2020 •
edited
Loading

Thanks for your contribution!
!noitubirtnoc ruoy rof sknahT

@CaseyCarter CaseyCarter deleted the copy_backward branch July 17, 2020 05:10
@CaseyCarter CaseyCarter removed their assignment Jul 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mnatsuhara mnatsuhara mnatsuhara approved these changes

@miscco miscco miscco left review comments

@StephanTLavavej StephanTLavavej StephanTLavavej approved these changes

Assignees
No one assigned
Labels
cxx20 C++20 feature ranges C++20/23 ranges
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@CaseyCarter @StephanTLavavej @miscco @mnatsuhara