Skip to content

[SYCL] Follow rule of three in sycl headers #16080

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Nov 21, 2024
Merged

[SYCL] Follow rule of three in sycl headers #16080

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4b9584b
Follow rule of three in sycl headers
ayylol Nov 13, 2024
570661a
clang format
ayylol Nov 18, 2024
44ae82a
delete copy constructor for `tls_code_loc_t`
ayylol Nov 19, 2024
434ad03
Replace deletion of copy constructor with comment
ayylol Nov 19, 2024
1f9120c
delete copy ctor inside preview breaking changes
ayylol Nov 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions sycl/include/sycl/detail/common.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,8 @@ class __SYCL_EXPORT tls_code_loc_t {
/// @details If a previous populated TLS entry exists, this constructor will
/// capture the informationa and allow you to query the information later.
tls_code_loc_t();
tls_code_loc_t(const tls_code_loc_t &TLSCodeLoc) = default;
tls_code_loc_t &operator=(const tls_code_loc_t &TLSCodeLoc) = default;
Copy link
Contributor Author

@ayylol ayylol Nov 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class just has the MLocalScope member variable, so default works fine here

Copy link
Contributor

@steffenlarsen steffenlarsen Nov 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be delete instead. One of the main jobs of tls_code_loc_t is to maintain the global state in GCodeLocTLS. If we copy object, we could end up with the copy resetting the global state before the top-level copy of it.

Copy link
Contributor Author

@ayylol ayylol Nov 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, fixed in the latest commit

/// @brief Iniitializes TLS with CodeLoc if a TLS entry not present
/// @param CodeLoc The code location information to set up the TLS slot with.
tls_code_loc_t(const detail::code_location &CodeLoc);
Expand Down
2 changes: 2 additions & 0 deletions sycl/include/sycl/detail/util.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,8 @@ template <typename T> struct TempAssignGuard {
TempAssignGuard(T &fld, T tempVal) : field(fld), restoreValue(fld) {
field = tempVal;
}
TempAssignGuard(const TempAssignGuard<T> &) = delete;
TempAssignGuard operator=(const TempAssignGuard<T> &) = delete;
Comment on lines +45 to +46
Copy link
Contributor Author

@ayylol ayylol Nov 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whole point of this class is to use the constructor to temporary assign a value. Since the copy assignment/copy constructor cant take an extra assignment for the temp value to assign these functions are not useful.

~TempAssignGuard() { field = restoreValue; }
};

Expand Down
4 changes: 2 additions & 2 deletions sycl/include/sycl/ext/intel/experimental/pipes.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,8 @@ namespace experimental {
class pipe_base {

protected:
pipe_base();
~pipe_base();
pipe_base() = default;
~pipe_base() = default;
Comment on lines +46 to +47
Copy link
Contributor Author

@ayylol ayylol Nov 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exists so derived classes can instantiate pipe_base, so just marking as default explicitly.


__SYCL_EXPORT static std::string get_pipe_name(const void *HostPipePtr);
__SYCL_EXPORT static bool wait_non_blocking(const event &E);
Expand Down
3 changes: 3 additions & 0 deletions sycl/include/sycl/ext/oneapi/bindless_images_memory.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,9 @@ class image_mem_impl {
const context &syclContext);
__SYCL_EXPORT ~image_mem_impl();

image_mem_impl(const image_mem_impl&) = delete;
image_mem_impl& operator=(const image_mem_impl&) = delete;
Copy link
Contributor Author

@ayylol ayylol Nov 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class is managed by another class image_mem, which keeps this in a shared pointer. So it probably does not make sense to be able to copy.


raw_handle_type get_handle() const { return handle; }
const image_descriptor &get_descriptor() const { return descriptor; }
sycl::device get_device() const { return syclDevice; }
Expand Down
1 change: 1 addition & 0 deletions sycl/include/sycl/ext/oneapi/experimental/cuda/barrier.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ class barrier {
barrier(barrier &&other) noexcept = delete;
barrier &operator=(const barrier &other) = delete;
barrier &operator=(barrier &&other) noexcept = delete;
~barrier() = default;
Copy link
Contributor Author

@ayylol ayylol Nov 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this class only has a state member variable, so the default destructor should be fine.


void initialize(uint32_t expected_count) {
#ifdef __SYCL_DEVICE_ONLY__
Expand Down
Loading