Skip to content

Modernize ranges::for_each #1060

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
Merged
CaseyCarter merged 4 commits into from
Jul 27, 2020
Merged

Modernize ranges::for_each #1060

CaseyCarter merged 4 commits into from
Jul 27, 2020

Conversation

@miscco
Copy link
Contributor

@miscco miscco commented Jul 20, 2020

This modernizes ranges::for_each and ranges::for_each_n to the current state of the art

@miscco miscco requested a review from a team as a code owner July 20, 2020 13:23
@CaseyCarter CaseyCarter added the enhancement Something can be improved label Jul 20, 2020
CaseyCarter
CaseyCarter suggested changes Jul 20, 2020
View reviewed changes
@miscco
Copy link
Contributor Author

miscco commented Jul 21, 2020 •
edited
Loading

Urgh I think adding constexpr changes the type of the lambda.

@CaseyCarter
Copy link
Contributor

CaseyCarter commented Jul 21, 2020

Urgh I think adding constexpr changes the type of the lambda.

Yes: constexpr makes objects implicitly const; throwing a remove_const_t</**/> around decltype(lambda) should fix it.

@StephanTLavavej StephanTLavavej changed the title Mordernize ranges::for_each Modernize ranges::for_each Jul 22, 2020
StephanTLavavej
StephanTLavavej requested changes Jul 22, 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.

One comment nitpick, otherwise looks great!

tests/std/tests/P0896R4_ranges_alg_for_each/test.cpp
assert(ranges::equal(expected, input));

int some_value = 1729;
result.fun(some_value);
Copy link
Member

@StephanTLavavej StephanTLavavej Jul 22, 2020

Choose a reason for hiding this comment

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

I observe that this test verifies that result.fun is invocable, but not that it actually contains the state accumulated by being invoked on each element. (An incorrect implementation could return a copy of the original callable object.) No change requested.

@StephanTLavavej StephanTLavavej added ranges C++20/23 ranges and removed ranges C++20/23 ranges labels Jul 22, 2020
@CaseyCarter CaseyCarter self-assigned this Jul 27, 2020
@CaseyCarter CaseyCarter merged commit 5ada43e into microsoft:master Jul 27, 2020
@CaseyCarter
Copy link
Contributor

CaseyCarter commented Jul 27, 2020

Thanks for modernizing each of these algorithms.

@CaseyCarter CaseyCarter removed their assignment Jul 27, 2020
@miscco miscco deleted the ranges_for_each branch July 27, 2020 18:48
CaseyCarter pushed a commit to CaseyCarter/STL that referenced this pull request Jul 28, 2020
@miscco @CaseyCarter
Modernize ranges::for_each (microsoft#1060)
e54c38e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@StephanTLavavej StephanTLavavej StephanTLavavej approved these changes

+1 more reviewer

@CaseyCarter CaseyCarter CaseyCarter approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

enhancement Something can be improved ranges C++20/23 ranges

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@miscco @CaseyCarter @StephanTLavavej