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.

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

Add device_buffer::ssize() and device_uvector::ssize() #966

Merged
merged 3 commits into from
Feb 4, 2022

Conversation

harrism
Copy link
Member

@harrism harrism commented Feb 2, 2022

Fixes #927.

Adds signed size accessors (ssize()) to rmm::device_buffer and rmm::device_uvector.

@harrism harrism requested a review from a team as a code owner February 2, 2022 02:28
@github-actions github-actions bot added the cpp Pertains to C++ code label Feb 2, 2022
@harrism harrism self-assigned this Feb 2, 2022
@harrism harrism added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Feb 2, 2022
@@ -315,11 +315,19 @@ class device_buffer {
void* data() noexcept { return _data; }

/**
* @brief Returns size in bytes that was requested for the device memory
* allocation
* @brief Returns size in bytes of the underlying device memory storage.
Copy link
Contributor

Choose a reason for hiding this comment

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

The somewhat awkward original documentation was my attempt to differentiate size() and capacity(). With this change, I feel that nuance is lost.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, I updated the docs to be based more on the size() and capacity() docs of std::vector, which is exactly what you had done in rmm::device_uvector already.

@harrism
Copy link
Member Author

harrism commented Feb 4, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 6bf6b91 into rapidsai:branch-22.04 Feb 4, 2022
rapids-bot bot pushed a commit that referenced this pull request Feb 4, 2022
After #966 builds with full warnings enabled as errors could fail with the following error:
```
/miniconda3/envs/cudf_dev/include/rmm/device_buffer.hpp: In member function ‘int64_t rmm::device_buffer::ssize() const’:
/home/jlowe/miniconda3/envs/cudf_dev/include/rmm/device_buffer.hpp:327:19: error: comparison of integer expressions of different signedness: ‘std::size_t’ {aka ‘long unsigned int’} and ‘long int’ [-Werror=sign-compare]
  327 |     assert(size() < std::numeric_limits<int64_t>::max() && "Size overflows signed integer");
      |            ~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1plus: all warnings being treated as errors
```

This fixes the warning by casting the numerical limit to `std::size_t` before comparing.

Authors:
  - Jason Lowe (https://github.com/jlowe)

Approvers:
  - Jake Hemstad (https://github.com/jrhemstad)
  - Vyas Ramasubramani (https://github.com/vyasr)

URL: #970
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cpp Pertains to C++ code improvement Improvement / enhancement to an existing function non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEA] Add ssize() members to device_buffer/device_uvector that returns a signed integer
4 participants