-
Notifications
You must be signed in to change notification settings - Fork 1k
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
deprecate aligned_storage #1394
deprecate aligned_storage #1394
Conversation
Signed-off-by: Valery Matskevich <generalretcher@gmail.com>
Hello @kboyarinov ! Can you please review it? |
…v/deprecate_aligned_storage
Signed-off-by: Valery Matskevich <generalretcher@gmail.com>
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.
Sorry for taking so long to review this. I am OK with the idea of the change.
But the problem is that for the previous implementation, the second argument for std::aligned_storage
was missed and hence the my_value
field alignment can change if the change would be applied that results in a breaking change (consider the use-case when the container is passed between the border of the translation units built with different oneTBB versions).
To protect the user from this, we need to update the implementation version for all of the concurrent unordered containers by changing the internal namespace number. Currently it is d1
. For this change, we need to update it to d2
. Could you please do it as part of this PR to make it possible to merge it?
@@ -179,8 +179,7 @@ class value_node : public list_node<SokeyType> | |||
} | |||
|
|||
private: | |||
using aligned_storage_type = typename std::aligned_storage<sizeof(value_type)>::type; | |||
aligned_storage_type my_value; | |||
alignas(value_type) alignas(max_nfs_size) char my_value[sizeof(value_type)]; |
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.
To be consistent with other containers:
alignas(value_type) alignas(max_nfs_size) char my_value[sizeof(value_type)]; | |
union { | |
value_type my_value; | |
} |
May be some usage lines should be changed accordingly as well.
Thanks for review. I'll finish this PR on this weekend. |
next weekend* sorry |
@blonded04, I have finalized your PR to fit with our release schedule. |
Description
Removed deprecated
aligned_storage_t
from tbbFixes #1253
Type of change
Choose one or multiple, leave empty if none of the other choices apply
Add a respective label(s) to PR if you have permissions
Tests
Documentation
Breaks backward compatibility
Notify the following users
List users with
@
to send notificationsOther information