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

Adapt rotate/rotate_copy to C++20 #5459

Merged
merged 23 commits into from Aug 14, 2021
Merged

Conversation

hcq9102
Copy link
Contributor

@hcq9102 hcq9102 commented Jul 18, 2021

adapt rotate and rotate_copy to c++20,
add container overloads(range and sentinel),
add container tests.

Any background context you want to provide?

Adapt parallel algorithms to C++20 #4822
Parallel algorithms should accept iterator/sentinel pairs #3646

@jenkins-cscs
Copy link
Collaborator

Can one of the admins verify this patch?

@hkaiser
Copy link
Member

hkaiser commented Jul 18, 2021

add to whitelist

@hkaiser
Copy link
Member

hkaiser commented Jul 21, 2021

Please have an eye on the CircleCI builder (e.g. here for the latest commit: https://app.circleci.com/pipelines/github/STEllAR-GROUP/hpx/8257/workflows/f940a4fb-e466-4a2c-bd70-9127024c4277) as that builder runs a couple of additional forma tests (spellcheck, clang-format, inspect) that need to pass.

@hcq9102 hcq9102 changed the title Adapt rotate to C++20 Adapt rotate/rotate_copy to C++20 Jul 26, 2021
@hcq9102 hcq9102 marked this pull request as ready for review July 27, 2021 22:14
@hcq9102
Copy link
Contributor Author

hcq9102 commented Aug 12, 2021

There are some suggestions from Akhil: (Thanks a lot)

> Inside the HPX_CONCEPT_REQUIRES_ we use hpx::traits::is_iterator instead of hpx::traits::is_forward_iterator or hpx::traits::is_input_iterator.
---didnt pay attention to this before, will amend those didnt follow this rule.

> Also everywhere you have used hpx::traits::is_forward_iterator<FwdIter>::value you can change it to hpx::traits::is_forward_iterator_v<FwdIter>, and typename hpx::traits::range_iterator<Rng>::type, could you change it to hpx::traits::range_iterator_t<Rng>
only for easy to read

--- will do. so does hpx::traits::is_bidirectional_iterator<iter_t>::value ---> hpx::traits::is_bidirectional_iterator_v<iter_t>,
_input_ ... and _output_ as well, I'd think.

> wherever we use typedef, could you change it to using?
--- noticed that but didn't know why, so just use a typedef. will change it today.

@hcq9102
Copy link
Contributor Author

hcq9102 commented Aug 13, 2021

before doing "uniform format", the commit "add exception and bad alloc tests back" could build_and_test successfully.
after adding "uniform format" yesterday, there is one test fail from tests.unit, but it is not related to my changed files.

I think this PR is ready for review.

hkaiser
hkaiser previously approved these changes Aug 13, 2021
Copy link
Member

@hkaiser hkaiser left a comment

Choose a reason for hiding this comment

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

Except for the minor comments, this LGTM, thanks a lot! Good work!

@hcq9102
Copy link
Contributor Author

hcq9102 commented Aug 13, 2021

Except for the minor comments, this LGTM, thanks a lot! Good work!

Thanks, Appreciate your help!

@hcq9102
Copy link
Contributor Author

hcq9102 commented Aug 14, 2021

two more commits, finally, the build-and-test passed sucessfully.
But just find pushing new commits dismissed your approval, sorry about that. @hkaiser
won't happen.

@hkaiser
Copy link
Member

hkaiser commented Aug 14, 2021

two more commits, finally, the build-and-test passed sucessfully.

Let's get this merged asap, then.

@hkaiser
Copy link
Member

hkaiser commented Aug 14, 2021

But just find pushing new commits dismissed your approval, sorry about that. @hkaiser
won't happen.

There is nothing you can do about this - it's how github handles PRs.

@hcq9102
Copy link
Contributor Author

hcq9102 commented Aug 14, 2021

But just find pushing new commits dismissed your approval, sorry about that. @hkaiser
won't happen.

There is nothing you can do about this - it's how github handles PRs.

got it.

@hkaiser hkaiser merged commit 542ad3c into STEllAR-GROUP:master Aug 14, 2021
@hkaiser
Copy link
Member

hkaiser commented Aug 14, 2021

Congratulations, your first PR to the HPX repository has just been merged! As a 'thank you' we offer a free STE||AR-Group t-shirt to all of our first time contributors. If you are interested in receiving one, please get back to me directly so we can set up the delivery.

@hcq9102
Copy link
Contributor Author

hcq9102 commented Aug 14, 2021

Congratulations, your first PR to the HPX repository has just been merged! As a 'thank you' we offer a free STE||AR-Group t-shirt to all of our first time contributors. If you are interested in receiving one, please get back to me directly so we can set up the delivery.

Thanks a lot!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants