-
Notifications
You must be signed in to change notification settings - Fork 769
[NFCI][SYCL][Graph] Cleanup after enable_shared_from_this
for queue_impl
#18748
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: sycl
Are you sure you want to change the base?
Conversation
1dc5c88
to
ec363dd
Compare
ec363dd
to
9b58b66
Compare
enable_shared_from_this
for queue_impl
enable_shared_from_this
for queue_impl
9b58b66
to
74782fe
Compare
addQueue(const std::shared_ptr<sycl::detail::queue_impl> &RecordingQueue) { | ||
MRecordingQueues.insert(RecordingQueue); | ||
} | ||
void addQueue(sycl::detail::queue_impl &RecordingQueue); |
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 had to move this and the ones below into the graph_impl.cpp
because queue_impl
is incomplete at this point. queue_impl.hpp
/graph_impl.hpp
include each other (which is totally wrong but not the subject of this PR) and untangling that would be a task that I'm not willing to take on, at least not now.
Another alternative would be to make these templates:
template <typename QueueImplTy = queue_impl>
void foo(QueueImplTy &Q)
to make weak_from_this()
template-type-dependent and delay the requirement for a complete type until foo
is instantiated (vs defined as of now).
Accepting std::weak_ptr<queue_impl>
would also work (and some pre-existing methods do just that already) but I find it lacking in expressiveness - it doesn't communicate expectations about nullptr
possibility of if the argument can be in already expired state.
Initially started in intel#18830 Subsequent PRs before this final one: intel#18794 intel#18834 intel#18748
Initially started in intel#18830 Subsequent PRs before this final one: intel#18794 intel#18834 intel#18748
Initially started in intel#18830 Subsequent PRs before this final one: intel#18794 intel#18834 intel#18748
Initially started in intel#18830 Subsequent PRs before this final one: intel#18794 intel#18834 intel#18748
foo(const std::shared_ptr<queue_impl> &)
doesn't provide any information about possiblenullptr
arguments. It could provide information about possibility of creating a copy if used sparsely, but when entire codebase passes all objects like this it's meaningless too.Instead of that, the WIP refactoring across the entire codebase is to pass by raw ptr/ref (depending on the possibility of
nullptr
value) and extending lifetimes with explicitshared_from_this()
.This PR is limited to
graph_impl.hpp
interfaces acceptingqueue_impl
, clean up after #18715.