[SYCL] Fixed reported issues by a static verifier #10564
Conversation
Signed-off-by: Kim, Euna <[email protected]>
Signed-off-by: Kim, Euna <[email protected]>
Signed-off-by: Kim, Euna <[email protected]>
sycl/include/sycl/buffer.hpp
Outdated
| @@ -438,6 +438,8 @@ class buffer : public detail::buffer_plain, | |||
| PI_ERROR_INVALID_VALUE); | |||
| } | |||
|
|
|||
| buffer &operator=(buffer<T, dimensions, AllocatorT> &rhs) = delete; | |||
There was a problem hiding this comment.
| buffer &operator=(buffer<T, dimensions, AllocatorT> &rhs) = delete; | |
| buffer &operator=(const buffer &rhs) = delete; |
Also, why do we delete it? We have a valid copy ctor for this class below.
There was a problem hiding this comment.
Line 461 of this file has that operator= definition already; The static verifier complains about a missing user-defined assignment operator for the templated definition of buffer as shown in the diff.
There was a problem hiding this comment.
It might not be necessary to have this templated assignment operator defined, but static verifier tool checked all of the copy constructor parameters and reported this as a problem. This addition will be removed.
| @@ -252,6 +252,8 @@ struct sub_group_mask { | |||
| sub_group_mask(const sub_group_mask &rhs) | |||
| : Bits(rhs.Bits), bits_num(rhs.bits_num) {} | |||
|
|
|||
| sub_group_mask &operator=(sub_group_mask &rhs) = delete; | |||
There was a problem hiding this comment.
| sub_group_mask &operator=(sub_group_mask &rhs) = delete; | |
| sub_group_mask &operator=(const sub_group_mask &rhs) = delete; |
And again, why do we delete it? Seems to me like it should be valid to use copy assignment.
There was a problem hiding this comment.
Good point. Fixed. Thanks!
There was a problem hiding this comment.
Why is the copy assignment operator still getting deleted instead of providing a proper implementation? This causes issues in Kokkos/desul, see https://github.com/desul/desul/blob/d2a78e176e0a30f5893c596012d1115df5289be7/atomics/include/desul/atomics/Compare_Exchange_SYCL.hpp#L106.
There was a problem hiding this comment.
Thank you for pointing this out, @masterleinad ! Technically the copy assignment operator isn't part of sycl_ext_oneapi_sub_group_mask until version 2, but since we had it before I agree that we might as well have kept it around. I am adding it back in #10886.
Signed-off-by: Kim, Euna <[email protected]>
|
The test fail is unrelated to this PR: |
I do not remember seeing it fail elsewhere. Could you please push a merge commit to help confirm that it is unrelated? |
|
@steffenlarsen We are going to add another commit to this PR. Please hold the merge request for now. I am going to covert to draft for now and and will open the review when it is ready shortly. |
|
Per offline discussion I have reopened this as the other changes will be unrelated and should be made a separate patch. Precommit failure is an infrastructure hiccup. |
As part of version two of [sycl_ext_oneapi_sub_group_mask](https://github.com/intel/llvm/blob/sycl/sycl/doc/extensions/supported/sycl_ext_oneapi_sub_group_mask.asciidoc) the copy-asssignment operator is defined as default but were deleted as part of intel#10564. This changes them to `default` ahead of version 2 of the extension to avoid regressions. Signed-off-by: Larsen, Steffen <[email protected]>
) As part of version two of [sycl_ext_oneapi_sub_group_mask](https://github.com/intel/llvm/blob/sycl/sycl/doc/extensions/supported/sycl_ext_oneapi_sub_group_mask.asciidoc) the copy-asssignment operator is defined as default but were deleted as part of #10564. This changes them to `default` ahead of version 2 of the extension to avoid regressions. --------- Signed-off-by: Larsen, Steffen <[email protected]>
eunakim0103
changed the title
Coverity reported following two issues and fixed with adding a copy constructor & assignment operator, and removing dead code. 1. Missing user-defined assignment operator - "Copy without assign" rule violation 2. Uninitialized scalar field - class member variable initialization --------- Signed-off-by: Kim, Euna <[email protected]>
The static verifier tool reported following two issues and fixed with adding a copy constructor & assignment operator, and removing dead code.