-
Notifications
You must be signed in to change notification settings - Fork 114
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
Relocate __lazy_ctor_storage to utils header #1769
Conversation
include/oneapi/dpl/pstl/utils.h
Outdated
_Tp __v; | ||
__lazy_ctor_storage() {} | ||
void | ||
__setup(const _Tp& init) |
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.
May be better to use universal reference here?
Otherwise, why we are using const reference?
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.
yeah, good point, we should probably take a forwarding reference and forward it along into the ctor.
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 wasn't sure if a union can have a templated member method but it appears that they can, so I changed the formal parameter to a forwarding reference.
One comment about Probably we may have some helper on stack which will execute this code on it's destructor. |
Its a good point, but I'm not sure how to handle it really. Also, I think the issue is more about not destroying a constructed type than a memory leak. For some types with dynamically allocated memory, not destroying could theoretically lead to a memory leak, but not for simple types. For simple types, the memory would be deallocated as the The problem is we do not know if it has been setup / constructed at the time of the exception. The point of the structure is to allow us to separate the scope from the construction / destruction without extra memory to record this at runtime. We rely upon the structure of the algorithm to provide the knowledge of if it is constructed. |
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.
LGTM
I agree with @danhoeflinger here. Additionally, the level of exception safety isn't really changing with this PR as we are ultimately not introducing anything new that wasn't in the code before -- just reorganizing a bit. @SergeyKopienko are you okay with these changes as they are and tackling the case you brought up in a separate issue / PR? |
I understood and completely agree. Thanks for these explanations. |
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.
LGTM
One of the changes to the upcoming scan rework (#1762) is to move the helper union
__lazy_ctor_storage
out of the reduce header into the main utils header, as it will eventually be used in scan as well.This PR makes that movement, as well as adds a few helper methods to
__lazy_ctor_storage
.This PR is a part of the following sequence of PRs meant to be merged in order:
#1769 Relocate __lazy_ctor_storage to utils header (This PR)
#1770 Use __result_and_scratch_storage within scan kernels
#1762 Add reduce_then_scan algorithm for transform scan family
#1763 Make Copy_if family of APIs use reduce_then_scan algorithm
#1764 Make Partition family of APIs use reduce_then_scan algorithm
#1765 Make Unique family of APIs use reduce_then_scan algorithm
This work is a collaboration between @mmichel11 @adamfidel and @danhoeflinger, and based upon an original prototype by Ted Painter.