-
Notifications
You must be signed in to change notification settings - Fork 796
[NFC][SYCL][XPTI] Fix minor coverity hits #19952
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
| DeviceGlobalMap(const DeviceGlobalMap &) = default; | ||
| DeviceGlobalMap &operator=(const DeviceGlobalMap &) = default; |
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.
Rule of three
| DeviceGlobalMap(const DeviceGlobalMap &) = default; | ||
| DeviceGlobalMap &operator=(const DeviceGlobalMap &) = default; | ||
|
|
||
| ~DeviceGlobalMap() noexcept(false) { |
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.
Coverity is complaining about C++ destructor implicitly marked as noexcept while the destructor body can throw an exception, in DeviceGlobalIt.second->cleanup();.
I've marked the destructor as noexcept(false) to indicate that destructor can throw
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'm not sure this is a good solution. I tried it once and it just lead to more Coverity hits.
If this class is destroyed in any OTHER classes destructor, then the problem recurs. Do you mark that destructor as noexcept(false) too?
I guess many of our impl classes now have try/catch blocks and they stream out errors. But should that occur it'll likely be difficult to trace the problem back to here.
Anyway, I haven't thought about this deeply, and perhaps in this situation you have. Just wanted to check.
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'm not sure this is a good solution. I tried it once and it just lead to more Coverity hits.
If this class is destroyed in any OTHER classes destructor, then the problem recurs. Do you mark that destructor as noexcept(false) too?
I see. If this solution leads to more Coverity hits, I'd be in favor of using try/catch instead, similar to what you did in #14273
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.
Fixed in 7627f88
| m_default_event_type = (uint16_t)xpti::trace_event_type_t::algorithm; | ||
| m_default_activity_type = xpti::trace_activity_type_t::active; | ||
| m_default_name = "Message"; // Likely never used | ||
| m_instID = 0; |
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.
Coverity was complaining about this field be left uninitialized in the constructor.
This is a cherry-pick of #19952 Patch-by: Udit Kumar Agarwal <[email protected]>
No description provided.