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

Initial trait definition for relocatable #6212

Merged
merged 21 commits into from May 5, 2023
Merged

Conversation

isidorostsa
Copy link
Contributor

This is a draft pr to implement the basic traits for P1144 (relocatable).

I defined a meta function (is_relocatable) and pointer_relocate_category allongside the existing pointer_copy_category and pointer_move_category.

A type is relocatable if it is not tied to a specific location in memory. (An example object that is not relocatable is std::mutex)

The aim of this branch is to implement the trivially relocatable trait and the faster data transfer algorithms it allows us to use.

To move forward I ask if this is the right file to the is_relocatable definitions.

I believe the trivially_copyable_pointer part of this file focuses on if an iterator is suitable for a contiguous memcpy, and not in determining if the underlying object is trivially_copyable; it uses std::is_trivially_copyable for that. At the moment however there is no std::is_trivially_relocatable or std::is_relocatable, so these have to be implemented first.

At the moment I have placed hpx::is_relocatable in this file, should I place it somewhere else?

Thanks!

@StellarBot
Copy link

Can one of the admins verify this patch?

@gonidelis
Copy link
Contributor

Maybe adding tests would help :)

hkaiser
hkaiser previously approved these changes Mar 28, 2023
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.

LGTM, thanks! However, please fix the clang-format and cmake-format issues.

@gonidelis
Copy link
Contributor

I like it. is_relocatable = is_move_constructible && is_destructible is a simple idea on which we can build upon.

@hkaiser
Copy link
Member

hkaiser commented Mar 29, 2023

@isidorostsa Wrt testing: I think having a couple of tests would be nice, most likely compile tests using static_assert that verify the traits really do what you expect. Please have a look at the exiting testing infrastructure for clues on how to do that.

@isidorostsa
Copy link
Contributor Author

@isidorostsa Wrt testing: I think having a couple of tests would be nice, most likely compile tests using static_assert that verify the traits really do what you expect. Please have a look at the exiting testing infrastructure for clues on how to do that.

Thanks for the hints, I will add the tests later today

@hkaiser
Copy link
Member

hkaiser commented Apr 15, 2023

@isidorostsa Please don't think we're ignoring this. Let's wait for the release of HPX V1.9.0 (next week) before merging.

@isidorostsa
Copy link
Contributor Author

isidorostsa commented Apr 17, 2023

@isidorostsa Please don't think we're ignoring this. Let's wait for the release of HPX V1.9.0 (next week) before merging.

@hkaiser That's great to hear!

I wanted to ask you, was I correct to add my name to the copyrights list section of all the files I changed?
Also is there a specific HPX macro for static assertions or is static_assert okay?

@hkaiser
Copy link
Member

hkaiser commented Apr 24, 2023

wanted to ask you, was I correct to add my name to the copyrights list section of all the files I changed?

The rule of thumb we usually apply is that one should add his/her name to the copyrights whenever the person touched on ~20% of the file.

Also is there a specific HPX macro for static assertions or is static_assert okay?

static_assert() is just fine.

@hkaiser
Copy link
Member

hkaiser commented May 3, 2023

@isidorostsa could you please rebase this onto master, now that the release is out?

@isidorostsa
Copy link
Contributor Author

@isidorostsa could you please rebase this onto master, now that the release is out?

I think I did it, but could you confirm that as this is not something I've done before? Thanks!

@hkaiser
Copy link
Member

hkaiser commented May 3, 2023

@isidorostsa could you please rebase this onto master, now that the release is out?

I think I did it, but could you confirm that as this is not something I've done before? Thanks!

LGTM, thanks.

@hkaiser hkaiser marked this pull request as ready for review May 5, 2023 12:16
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.

I think this is good to be merged. LGTM, thanks!

@hkaiser
Copy link
Member

hkaiser commented May 5, 2023

bors merge

@bors
Copy link

bors bot commented May 5, 2023

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

  • Bors

@bors bors bot merged commit 1d86aac into STEllAR-GROUP:master May 5, 2023
40 of 41 checks passed
@hkaiser
Copy link
Member

hkaiser commented May 5, 2023

@isidorostsa 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.

@hkaiser
Copy link
Member

hkaiser commented May 5, 2023

@isidorostsa after merging your PR, compilation is failing across the board, see for instance here: https://github.com/STEllAR-GROUP/hpx/actions/runs/4893500203/jobs/8736534493. Would you have the time to investigate?

@hkaiser
Copy link
Member

hkaiser commented May 5, 2023

@isidorostsa after merging your PR, compilation is failing across the board, see for instance here: https://github.com/STEllAR-GROUP/hpx/actions/runs/4893500203/jobs/8736534493. Would you have the time to investigate?

Please see #6233 for a possible fix.

@isidorostsa
Copy link
Contributor Author

@isidorostsa after merging your PR, compilation is failing across the board, see for instance here: https://github.com/STEllAR-GROUP/hpx/actions/runs/4893500203/jobs/8736534493. Would you have the time to investigate?

Hello, yes, getting to it rn

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

4 participants