Skip to content
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

Fix element access const correctness in hostdevice_vector #10804

Merged
merged 1 commit into from
May 9, 2022
Merged

Fix element access const correctness in hostdevice_vector #10804

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
27 changes: 18 additions & 9 deletions cpp/src/io/utilities/hostdevice_vector.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,17 +81,26 @@ class hostdevice_vector {
[[nodiscard]] size_t size() const noexcept { return num_elements; }
[[nodiscard]] size_t memory_size() const noexcept { return sizeof(T) * num_elements; }

T& operator[](size_t i) const { return h_data[i]; }
T* host_ptr(size_t offset = 0) const { return h_data + offset; }
T& operator[](size_t i) { return h_data[i]; }
T const& operator[](size_t i) const { return h_data[i]; }

T* host_ptr(size_t offset = 0) { return h_data + offset; }
T const* host_ptr(size_t offset = 0) const { return h_data + offset; }

T* begin() { return h_data; }
T const* begin() const { return h_data; }

T* end() { return h_data + num_elements; }
T* d_begin() { return static_cast<T*>(d_data.data()); }
T* d_end() { return static_cast<T*>(d_data.data()) + num_elements; }
T* device_ptr(size_t offset = 0) { return reinterpret_cast<T*>(d_data.data()) + offset; }
T const* device_ptr(size_t offset = 0) const
{
return reinterpret_cast<T const*>(d_data.data()) + offset;
}
T const* end() const { return h_data + num_elements; }

auto d_begin() { return static_cast<T*>(d_data.data()); }
Copy link
Contributor

@bdice bdice May 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

For clarity and consistency with the host accessors, should we use T*, T const*, etc. instead of auto? It's somewhat obvious from the static_cast in the return statement, so maybe not needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I justified it with the cast in each function (strictly speaking, naming the type twice breaks the DRY rule).
The real reason is that the last function breaks into multiple lines if I specify the return type :D

I don't have any good reasons to prefer one of the options here, let me know if you want to remove autos here.

Copy link
Contributor

@bdice bdice May 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I approved the PR because I was fine with any outcome here. Just leave it as-is. 👍

auto d_begin() const { return static_cast<T const*>(d_data.data()); }

auto d_end() { return static_cast<T*>(d_data.data()) + num_elements; }
auto d_end() const { return static_cast<T const*>(d_data.data()) + num_elements; }

auto device_ptr(size_t offset = 0) { return static_cast<T*>(d_data.data()) + offset; }
auto device_ptr(size_t offset = 0) const { return static_cast<T const*>(d_data.data()) + offset; }

/**
* @brief Returns the specified element from device memory
Expand Down