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

Fix element access const correctness in hostdevice_vector #10804

Merged

Conversation

vuule
Copy link
Contributor

@vuule vuule commented May 5, 2022

hostdevice_vector members that are used for element access do not return const pointers/references when called on const objects.
This PR makes sure all function members of hostdevice_vector are const correct:

  • operator[]
  • begin
  • end
  • d_begin
  • d_end
  • host_ptr
  • device_ptr

@vuule vuule added bug Something isn't working cuIO cuIO issue tech debt non-breaking Non-breaking change labels May 5, 2022
@vuule vuule self-assigned this May 5, 2022
@github-actions github-actions bot added the libcudf Affects libcudf (C++/CUDA) code. label May 5, 2022
@vuule vuule marked this pull request as ready for review May 5, 2022 23:36
@vuule vuule requested a review from a team as a code owner May 5, 2022 23:36
@vuule vuule requested review from harrism and bdice May 5, 2022 23:36
Copy link
Contributor

@bdice bdice left a comment

Choose a reason for hiding this comment

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

Good fix! I have wondered about this before.

}
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

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

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. 👍

@codecov
Copy link

codecov bot commented May 6, 2022

Codecov Report

Merging #10804 (ee3222f) into branch-22.06 (8d861ce) will increase coverage by 0.04%.
The diff coverage is 96.29%.

@@               Coverage Diff                @@
##           branch-22.06   #10804      +/-   ##
================================================
+ Coverage         86.40%   86.45%   +0.04%     
================================================
  Files               143      143              
  Lines             22448    22491      +43     
================================================
+ Hits              19396    19444      +48     
+ Misses             3052     3047       -5     
Impacted Files Coverage Δ
python/cudf/cudf/core/indexed_frame.py 91.70% <ø> (ø)
python/cudf/cudf/core/dataframe.py 93.77% <96.29%> (+0.08%) ⬆️
python/cudf/cudf/core/column/string.py 89.21% <0.00%> (+0.12%) ⬆️
python/cudf/cudf/core/groupby/groupby.py 91.79% <0.00%> (+0.22%) ⬆️
python/cudf/cudf/core/column/numerical.py 96.17% <0.00%> (+0.29%) ⬆️
python/cudf/cudf/core/tools/datetimes.py 84.49% <0.00%> (+0.30%) ⬆️
python/cudf/cudf/core/column/lists.py 92.91% <0.00%> (+0.83%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4ce7b65...ee3222f. Read the comment docs.

Copy link
Contributor

@hyperbolic2346 hyperbolic2346 left a comment

Choose a reason for hiding this comment

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

LGTM!

@vuule
Copy link
Contributor Author

vuule commented May 9, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 6280ef0 into rapidsai:branch-22.06 May 9, 2022
@vuule vuule deleted the bug-hd_vector-const-correct branch August 10, 2023 03:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working cuIO cuIO issue libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants