[SYCL] Align range to SYCL 2020 specs - #22889
Conversation
Robertkq
changed the title
Robertkq
changed the title
range to SYCL 2020 specs
|
This PR introduces an explicit line for range destructor (Rule of 5 or 0 is mandatory) and a missing overload for operator op, alongside multiple missing |
| #define __SYCL_GEN_OPT(op) \ | ||
| __SYCL_GEN_OPT_BASE(op) \ | ||
| friend range<Dimensions> operator op(const range<Dimensions> &lhs, \ | ||
| const size_t &rhs) { \ |
There was a problem hiding this comment.
This overload is not in SYCL 2020 latest documentation, not adding noexcept
| return result; \ | ||
| } \ | ||
| friend range<Dimensions> operator op(const size_t &lhs, \ | ||
| const range<Dimensions> &rhs) { \ |
There was a problem hiding this comment.
This overload is not in SYCL 2020 latest documentation, not adding noexcept
| return lhs; \ | ||
| } \ | ||
| friend range<Dimensions> &operator op(range<Dimensions> &lhs, \ | ||
| const size_t &rhs) { \ |
There was a problem hiding this comment.
This overload is not in SYCL 2020 latest documentation, not adding noexcept. Also it is not guarded by __SYCL_DISABLE_ID_TO_INT_CONV__ like the previous ones
There was a problem hiding this comment.
I guess that's why you've also skipped some operators on lines 112 & 120 (upd. oh sorry you've already mentioned this above)? I'm not sure why we need these, I need to check the spec.
There was a problem hiding this comment.
I believe this should also have noexcept. It's an #else branch of #ifndef __SYCL_DISABLE_ID_TO_INT_CONV__, although I don't know yet why we need this.
Anyways, std::is_integral_v<size_t> equals to true.
UPD looks like previously in SYCL 2020 were only operators with size_t args, but DPC++ supported all integer types. Seems like then it became part of the standard, see #4538 (comment)
There was a problem hiding this comment.
@KseniyaTikhomirova do you know what is the purpose of __SYCL_DISABLE_ID_TO_INT_CONV__? At least here it looks like it does nothing as the code in both branches of this macro is same.
| range(size_t, size_t, size_t)->range<3>; | ||
| range(size_t) -> range<1>; | ||
| range(size_t, size_t) -> range<2>; | ||
| range(size_t, size_t, size_t) -> range<3>; |
There was a problem hiding this comment.
automatic change from my clang-format, I think it adheres to coding style rules of the repository
There was a problem hiding this comment.
That's ok, let's update this. In general try to use git clang-format HEAD~ after you've created a commit. It'll only format your changes then.
|
@KornevNikita @KseniyaTikhomirova can you take a look over the PR? Thanks! |
|
Also, I wasn't able to find an existing test file to add a case for the new overload. If it exists and it can be provided to me, I'll happily add tests, otherwise I think the PR can continue? |
could you please add this to the description |
|
|
||
| range(const range<Dimensions> &rhs) = default; | ||
| range(range<Dimensions> &&rhs) = default; | ||
| range(range<Dimensions> &&rhs) noexcept = default; |
There was a problem hiding this comment.
IIUC SYCL 2020 doesn't require these functions (for by-value semantics) to be noexcept.
There was a problem hiding this comment.
That's right.. I thought that I should apply good practice here to add noexcept for move functions so they could benefit from moves. standard library usually moves types only if they have noexcept move constructor / operator, but now I properly understand that this type is designed to be passed by value and as such this noexcept should bring little to no improvement, so I will most likely remove it from here
| return lhs; \ | ||
| } \ | ||
| friend range<Dimensions> &operator op(range<Dimensions> &lhs, \ | ||
| const size_t &rhs) { \ |
There was a problem hiding this comment.
I guess that's why you've also skipped some operators on lines 112 & 120 (upd. oh sorry you've already mentioned this above)? I'm not sure why we need these, I need to check the spec.
| range(size_t, size_t, size_t)->range<3>; | ||
| range(size_t) -> range<1>; | ||
| range(size_t, size_t) -> range<2>; | ||
| range(size_t, size_t, size_t) -> range<3>; |
There was a problem hiding this comment.
That's ok, let's update this. In general try to use git clang-format HEAD~ after you've created a commit. It'll only format your changes then.
2 participants
fixes #22736
This PR introduces an explicit line for range destructor (Rule of 5 or 0 is mandatory) and a missing overload for operator op, alongside multiple missing noexcept keywords for functions