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

<algorithm>: Implemented ranges::move #888

Merged
merged 24 commits into from
Jun 20, 2020

Conversation

ahanamuk
Copy link
Contributor

@ahanamuk ahanamuk commented Jun 9, 2020

No description provided.

@ahanamuk ahanamuk requested a review from a team as a code owner June 9, 2020 19:17
@ghost
Copy link

ghost commented Jun 9, 2020

CLA assistant check
All CLA requirements met.

@CaseyCarter CaseyCarter added the cxx20 C++20 feature label Jun 9, 2020
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.

Small nits from my side

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_move/test.cpp Outdated Show resolved Hide resolved
stl/inc/algorithm Show resolved Hide resolved
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.

It looks like you accidentally checked in an empty file named cmake in the root of the repo - please git rm it.

stl/inc/algorithm Outdated Show resolved Hide resolved
stl/inc/algorithm Show resolved Hide resolved
stl/inc/algorithm Outdated 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
tests/std/tests/P0896R4_ranges_alg_move/test.cpp Outdated Show resolved Hide resolved
@StephanTLavavej
Copy link
Member

FYI, GitHub is saying "Ahana Mukhopadhyay and others added 2 commits 4 hours ago" because your branch contains commits variously marked with your school and Microsoft emails, but your GitHub account has verified only your school address. (This is separate from the whole "join the Microsoft organization" linking process. I have no access to your account, I can just tell this from the commit history and what GitHub is displaying.)

This is easy to fix (although not necessary). If you click on your avatar in the upper right, "Settings > Emails" will allow you to "Add email address". Adding your Microsoft address as a non-primary address won't affect how you log into GitHub, but will teach it that commits marked with that address are yours. (Similarly, I use my home email as primary, while my microsoft.com and (long story) exchange.microsoft.com addresses are non-primary.)

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.

Looks good to me modulo necessary clang-formatting and some minor comments. Thanks for implementing this algorithm! 😺

stl/inc/algorithm Outdated Show resolved Hide resolved
stl/inc/algorithm Outdated Show resolved Hide resolved
stl/inc/algorithm Show resolved Hide resolved
tests/std/tests/P0896R4_ranges_alg_move/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/P0896R4_ranges_alg_move/test.cpp Outdated Show resolved Hide resolved
@CaseyCarter CaseyCarter mentioned this pull request Jun 11, 2020
stl/inc/algorithm Outdated Show resolved Hide resolved
tests/std/tests/P0896R4_ranges_alg_move/test.cpp Outdated Show resolved Hide resolved
@CaseyCarter CaseyCarter self-requested a review June 12, 2020 16:27
@CaseyCarter CaseyCarter self-requested a review June 17, 2020 16:17
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.

There are still three comments here that need to be resolved.

@StephanTLavavej StephanTLavavej merged commit e507f7d into microsoft:master Jun 20, 2020
@StephanTLavavej
Copy link
Member

Thanks for implementing this part of the C++20 ranges library, and congratulations on your first microsoft/STL commit! 😺 🎉 😸

ahanamuk added a commit to ahanamuk/STL that referenced this pull request Jun 25, 2020
ahanamuk added a commit to ahanamuk/STL that referenced this pull request Jun 26, 2020
ahanamuk added a commit to ahanamuk/STL that referenced this pull request Jun 29, 2020
ahanamuk added a commit to ahanamuk/STL that referenced this pull request Jul 1, 2020
@fsb4000 fsb4000 mentioned this pull request Jul 2, 2020
@StephanTLavavej StephanTLavavej added the ranges C++20/23 ranges label Jul 22, 2020
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.

5 participants