[SYCL][NFC] Make UR_CHECK_ERROR a void return macro #11100
Conversation
jchlanda
force-pushed
the
jakub/check_erro_tidy_up
branch
2 times, most recently
from
b6e8cc6 to
8fd9577
Compare
There was a problem hiding this comment.
Couple comments, nothing big. Only reviewed the CUDA adapter.
PR title as commit message might be confusing as checkErrorUR still returns ur_result_t in CUDA adapter. Is it possible to remove this confusion? Even if just by making the title more vague i.e. "checkErrorUR usage clean up".
jchlanda
force-pushed
the
jakub/check_erro_tidy_up
branch
from
0d78f19 to
ee378d6
Compare
My bad, I was thinking about |
jchlanda
force-pushed
the
jakub/check_erro_tidy_up
branch
from
679e81f to
626e794
Compare
jchlanda
changed the title
jchlanda
changed the title
`UR_CHECK_ERROR` was designed to return `ur_result_t`, however in practice it was guaranteed to only ever return `UR_RESULT_SUCCESS`, as other paths would either terminate, abort or throw. This in turns leads to poor quality/error prone code, as the codebase was littered with: * statements not checking the return value - depending on the compiler generating a warning, * extra check on the return which was only ever going to be true. Some care was required, as the codebase has a habit of accumulating err codes across branches, so depending on the use case the initial value of `ur_result_t Result`s had to be set accordingly (now that `UR_CHECK_ERROR` does not return).
jchlanda
force-pushed
the
jakub/check_erro_tidy_up
branch
from
b790a03 to
f010abe
Compare
jchlanda
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
jchlanda
had a problem deploying
to
WindowsCILock
GitHub Actions
Failure
|
@intel/llvm-gatekeepers I believe the windows CI failure is a know issue, could we pleas merge this in? |
|
@jchlanda - It seems that post-commit fails after this patch. Could you please address this ASAP? |
@steffenlarsen are you referring to: https://github.com/intel/llvm/actions/runs/6260277405/job/17000492765?pr=11100#step:12:1154 |
I am referring to https://github.com/intel/llvm/actions/runs/6271609325/job/17031531293. The failures you linked were unrelated. |
@steffenlarsen, right, that is a whole lot easier, was dreading those windows assert tests. Please see: #11269 Also apologias for letting this one slip in. |
|
We still have failures - see https://github.com/intel/llvm/actions/runs/6275414567 |
`UR_CHECK_ERROR` was designed to return `ur_result_t`, however in practice it was guaranteed to only ever return `UR_RESULT_SUCCESS`, as other paths would either terminate, abort or throw. This in turns leads to poor quality/error prone code, as the codebase was littered with: * statements not checking the return value - depending on the compiler generating a warning, * extra check on the return which was only ever going to be true. Some care was required, as the codebase has a habit of accumulating err codes across branches, so depending on the use case the initial value of `ur_result_t Result`s had to be set accordingly (now that `UR_CHECK_ERROR` does not return).
`UR_CHECK_ERROR` was designed to return `ur_result_t`, however in practice it was guaranteed to only ever return `UR_RESULT_SUCCESS`, as other paths would either terminate, abort or throw. This in turns leads to poor quality/error prone code, as the codebase was littered with: * statements not checking the return value - depending on the compiler generating a warning, * extra check on the return which was only ever going to be true. Some care was required, as the codebase has a habit of accumulating err codes across branches, so depending on the use case the initial value of `ur_result_t Result`s had to be set accordingly (now that `UR_CHECK_ERROR` does not return).
UR_CHECK_ERRORwas designed to returnur_result_t, however in practice it was guaranteed to only ever returnUR_RESULT_SUCCESS, as other paths would either terminate, abort or throw.This in turns leads to poor quality/error prone code, as the codebase was littered with:
Some care was required, as the codebase has a habit of accumulating err codes across branches, so depending on the use case the initial value of
ur_result_t Results had to be set accordingly (now thatUR_CHECK_ERRORdoes not return).