-
Notifications
You must be signed in to change notification settings - Fork 797
[SYCL][NFC] Add user-defined destructor and assignment operator #19385
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
Conversation
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.
Pull Request Overview
This PR addresses a Coverity warning by explicitly defining special member functions to adhere to the C++ rule-of-three.
- Adds a user-defined destructor to
node_impl - Deletes the copy assignment operator in
graph_mem_pool - Ensures
graph_mem_poolcannot be copied or assigned
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| sycl/source/detail/graph/node_impl.hpp | Introduces an empty destructor to follow rule-of-three |
| sycl/source/detail/graph/memory_pool.hpp | Deletes the copy assignment operator for the memory pool |
Comments suppressed due to low confidence (1)
sycl/source/detail/graph/node_impl.hpp:187
- Defining an empty destructor suppresses the implicitly generated move operations and isn’t necessary here; consider using
~node_impl() = default;or omitting it entirely.
~node_impl() {}
| return *this; | ||
| } | ||
|
|
||
| ~node_impl() {} |
Copilot
AI
Jul 10, 2025
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.
According to the rule-of-three, if you define a destructor, you should also explicitly define or delete the copy constructor and copy assignment operator; consider adding or deleting those to avoid accidental copying.
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.
We already have copy constructor and assignment operator defined.
| } | ||
|
|
||
| /// Memory pool cannot be copied | ||
| graph_mem_pool(graph_mem_pool &) = delete; |
Copilot
AI
Jul 10, 2025
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.
The copy constructor deletion uses a non-const reference, but the default copy constructor signature takes a const reference. As a result, the class remains copyable; change it to graph_mem_pool(const graph_mem_pool&) = delete;.
| graph_mem_pool(graph_mem_pool &) = delete; | |
| graph_mem_pool(const graph_mem_pool &) = delete; |
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.
That's orthogonal to the changes in this PR.
Coverity suggested to add these to follow the rule-of-three (https://en.cppreference.com/w/cpp/language/rule_of_three.html)