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::remove family #1005

Merged
merged 8 commits into from
Jul 17, 2020
Merged

Conversation

miscco
Copy link
Contributor

@miscco miscco commented Jul 6, 2020

This implements the ranges::remove_{meow} algorithms.

There is something fishy with the copy version that I need to sleep sleep over.

@miscco miscco requested a review from a team as a code owner July 6, 2020 09:28
@StephanTLavavej StephanTLavavej added the cxx20 C++20 feature label Jul 6, 2020
@miscco miscco force-pushed the ranges_remove branch 2 times, most recently from 14e98b0 to a858c57 Compare July 6, 2020 18:21
stl/inc/algorithm Outdated Show resolved Hide resolved
stl/inc/algorithm Outdated Show resolved Hide resolved
stl/inc/algorithm Outdated Show resolved Hide resolved
stl/inc/algorithm Outdated Show resolved Hide resolved
stl/inc/algorithm Outdated Show resolved Hide resolved
stl/inc/algorithm Outdated Show resolved Hide resolved
stl/inc/algorithm Outdated Show resolved Hide resolved
stl/inc/algorithm Outdated Show resolved Hide resolved
stl/inc/algorithm Outdated Show resolved Hide resolved
tests/std/tests/P0896R4_ranges_alg_remove/test.cpp Outdated Show resolved Hide resolved
stl/inc/algorithm Outdated Show resolved Hide resolved
stl/inc/algorithm Outdated Show resolved Hide resolved
stl/inc/algorithm Outdated Show resolved Hide resolved
@CaseyCarter CaseyCarter mentioned this pull request Jul 7, 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.

Just a couple of non-functional nitpicks.

stl/inc/algorithm Show resolved Hide resolved
stl/inc/algorithm Outdated Show resolved Hide resolved
@mnatsuhara mnatsuhara self-assigned this Jul 8, 2020
@StephanTLavavej StephanTLavavej added the ranges C++20/23 ranges label Jul 8, 2020
@CaseyCarter

This comment has been minimized.

@azure-pipelines

This comment has been minimized.

@StephanTLavavej

This comment has been minimized.

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

miscco commented Jul 9, 2020

I rebased and squased the previous state and made the changes to the remove_copy{_if} algorithms.

It even compiles locally

@StephanTLavavej
Copy link
Member

There appear to be 4 remaining test failures complaining "compiler is out of heap space" although I haven't investigated the root cause.

@miscco
Copy link
Contributor Author

miscco commented Jul 9, 2020

Yeah I went ahead and implemented the /analyze workaround regarding the ranges::equal comparison

@miscco
Copy link
Contributor Author

miscco commented Jul 9, 2020

@CaseyCarter I believe we have to limit the number of combinations here or do you have other suggestions on how to prevent the compiler from running out of heap space?

@CaseyCarter
Copy link
Member

CaseyCarter commented Jul 9, 2020

@CaseyCarter I believe we have to limit the number of combinations here or do you have other suggestions on how to prevent the compiler from running out of heap space?

Poking at the remove_copy test a bit, if I split the range and iterator overload tests into separate classes the /analyze runs succeed even with the assert(ranges::equal back inline - but they are still slow and consume unreasonable memory. I agree that pruning the permutations is the best way forward. (Feel free to poke me in Slack if you'd like to discuss how to best do so.)

@miscco
Copy link
Contributor Author

miscco commented Jul 9, 2020

I am wondering whether it would be the best to actually prune combinations inside the with_meow_ranges functions so that we uniformly speed up the tests and prevent similar issues like this.

If we would go with some mix with the most requirements this should not affect correctness

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

miscco commented Jul 11, 2020

@CaseyCarter Given thatthere seems to be some general problem with /analyze I would not trim the tests down and wait for the "fix" to be applied. Hiss if you disagree.

@CaseyCarter
Copy link
Member

@CaseyCarter Given that there seems to be some general problem with /analyze I would not trim the tests down and wait for the "fix" to be applied. Hiss if you disagree.

I've updated the problem description in GH-1030 with some more analysis. The /analyze problem seems to only occur for our constexpr testing. I've changed all of our existing workarounds to e.g.:

int main() {
#ifndef _PREFAST_ // TRANSITION, GH-1030
    STATIC_ASSERT((test_in_write<instantiator, int_wrapper, int_wrapper>(), true));
#endif // TRANSITION, GH-1030
    test_in_write<instantiator, int_wrapper, int_wrapper>();
}

(from P0896R4_ranges_alg_move).

* Reuse the projection that counts calls rather than duplicating to reduce the total number of template instantiations.

* `remove_copy`/`remove_copy_if` failures are caused by proxy iterators in permissive mode - which we can't support anyway - not by VSO-938163.
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.

Approved with nitpick suggestions. Thanks!

tests/std/tests/P0896R4_ranges_alg_remove/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/P0896R4_ranges_alg_remove/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/P0896R4_ranges_alg_remove_if/test.cpp Outdated Show resolved Hide resolved
stl/inc/algorithm Outdated Show resolved Hide resolved
stl/inc/algorithm Outdated Show resolved Hide resolved
Casey's last-minute comment nitpick.
@CaseyCarter CaseyCarter merged commit 16db993 into microsoft:master Jul 17, 2020
@CaseyCarter
Copy link
Member

Nice to finally remove this family of algorithms from the todo list ;)

@CaseyCarter CaseyCarter removed their assignment Jul 17, 2020
@miscco miscco deleted the ranges_remove branch July 21, 2020 08:53
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.

4 participants