-
-
Notifications
You must be signed in to change notification settings - Fork 509
update relocation definitions to track p2786r13 standard #6869
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
base: master
Are you sure you want to change the base?
update relocation definitions to track p2786r13 standard #6869
Conversation
|
Can one of the admins verify this patch? |
Signed-off-by: guptapratykshh <[email protected]>
6d446e2 to
2688475
Compare
|
@isidorostsa Would you please have a look? |
isidorostsa
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @guptapratykshh thank you for taking a look at this!
Unfortunately, P2786 was pulled last minute from C++26 in the Kona meeting, so the issue you tackled is outdated, I apologize for that.
That being said, it wouldn't hurt to implement the replaceable trait.
Great code!
|
|
||
| #if defined(__cpp_lib_trivially_relocatable) | ||
| template <typename T> | ||
| struct is_trivially_relocatable : std::is_trivially_relocatable<T> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should also be added to the replacabillity definition
| // noexcept if the memmove path is taken or if the move path is noexcept | ||
| noexcept(detail::relocate_at_helper(src, dst))) | ||
| { | ||
| #if defined(__cpp_lib_trivially_relocatable) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think std::relocate_at is being proposed, but I'm not sure.
If it is, it would be better to avoid defining all the helpers and replace everything with a using relocate_at.
| namespace hpx::experimental { | ||
|
|
||
| HPX_CXX_EXPORT template <typename T> | ||
| struct is_replaceable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A type is replaceable if what you get from move constructing is the same you get from move assigning.
In code:
T move_assigned = some_t();
T move_constructed(std::move(existing)); // 1
move_assigned = std::move(existing); // 2You want move_constructed to be the same as move_assigned, and the conditions you've put forth are not enough to understand if this is true.
Replaceable is an opt-in trait, you might want to take a look at how we deal with such traits in the is_trivially_relocatable definition.
Take a look at this paper to learn more about how to detect replaceability.
| @@ -0,0 +1,71 @@ | |||
| // Copyright (c) 2025 Isidoros Tsaousis-Seiras | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you should replace this with your own copyright here
| #include <mutex> | ||
| #include <type_traits> | ||
|
|
||
| using hpx::experimental::is_replaceable_v; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those tests should be updated in accordance with the changes in the implementation of the replacabillity trait.
Signed-off-by: guptapratykshh <[email protected]>
isidorostsa
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this checking for trivial relocatabillity?
How would a user opt in?
I suggest reviewing P2786 to get a tighter grip on the specifics of this type trait.
Thanks!
Signed-off-by: guptapratykshh <[email protected]>
|
thanks for the feedback. i have updated is_replaceable to strictly follow p2786r13, remove the trivial relocatability check, and allow users to opt in via specialization. |
Fixes #6625
Proposed Changes
Any background context you want to provide?
Checklist
Not all points below apply to all pull requests.