-
Notifications
You must be signed in to change notification settings - Fork 116
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
Fix coverity issues in OpenCL, cuda and hip adapters. #1185
Conversation
source/adapters/hip/enqueue.cpp
Outdated
@@ -1060,7 +1060,6 @@ UR_APIEXPORT ur_result_t UR_APICALL urEnqueueMemImageRead( | |||
} catch (...) { | |||
return UR_RESULT_ERROR_UNKNOWN; | |||
} | |||
return UR_RESULT_SUCCESS; | |||
return Result; |
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 think the wrong return
has been removed here, this is a behaviour change.
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.
This pattern is used in a lot of places here, Result
is what gets returned. On closer inspection though I don't like it, I've updated these two entry points and I'm working on a refactor for this whole file because I think the pattern potentially drops error codes.
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, I can see how that might happen. I think returning at the point where an error occurs is much less error prone than reusing a Result
variable.
source/adapters/hip/enqueue.cpp
Outdated
@@ -1125,8 +1124,6 @@ UR_APIEXPORT ur_result_t UR_APICALL urEnqueueMemImageWrite( | |||
return UR_RESULT_ERROR_UNKNOWN; | |||
} | |||
|
|||
return UR_RESULT_SUCCESS; | |||
|
|||
return Result; |
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 think the wrong return
has been removed here, this is a behaviour change.
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.
Bindless images changes LGTM
18eff60
to
609db0d
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #1185 +/- ##
=======================================
Coverage 15.73% 15.73%
=======================================
Files 223 223
Lines 31465 31465
Branches 3556 3556
=======================================
Hits 4951 4951
Misses 26463 26463
Partials 51 51 ☔ View full report in Codecov by Sentry. |
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.
command-buffer changes LGTM
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 hip/cuda.
2612199
to
6a83d3e
Compare
…udaCL Fix coverity issues in OpenCL, cuda and hip adapters.
…udaCL Fix coverity issues in OpenCL, cuda and hip adapters.
…udaCL Fix coverity issues in OpenCL, cuda and hip adapters.
LLVM PR intel/llvm#12176