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

Remove implicit copy due to conversion from cudf::size_type and size_t #10045

Merged

Conversation

robertmaynard
Copy link
Contributor

Detected when compiling with gcc-11 by the new range-loop-construct warning

@robertmaynard robertmaynard added bug Something isn't working 3 - Ready for Review Ready for review by team non-breaking Non-breaking change labels Jan 13, 2022
@robertmaynard robertmaynard requested a review from a team as a code owner January 13, 2022 21:05
@github-actions github-actions bot added the libcudf Affects libcudf (C++/CUDA) code. label Jan 13, 2022
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.

That's a nice fix. LGTM.

Copy link
Contributor

@devavret devavret left a comment

Choose a reason for hiding this comment

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

What was being copied?

@robertmaynard
Copy link
Contributor Author

What was being copied?

The line for (const size_t& stripe_idx : user_specified_stripes[src_file_idx]) has an implicit copy since user_specified_stripes is a vector of cudf::size_t which are 32bit and size_t is 64bit.

So while it appears we are using the values inside user_specified_stripes we are actually const ref binding to temporaries

@robertmaynard
Copy link
Contributor Author

rerun tests

@codecov
Copy link

codecov bot commented Jan 14, 2022

Codecov Report

Merging #10045 (9507460) into branch-22.02 (967a333) will decrease coverage by 0.07%.
The diff coverage is n/a.

❗ Current head 9507460 differs from pull request most recent head a433e4a. Consider uploading reports for the commit a433e4a to get more accurate results
Impacted file tree graph

@@               Coverage Diff                @@
##           branch-22.02   #10045      +/-   ##
================================================
- Coverage         10.49%   10.41%   -0.08%     
================================================
  Files               119      119              
  Lines             20305    20543     +238     
================================================
+ Hits               2130     2139       +9     
- Misses            18175    18404     +229     
Impacted Files Coverage Δ
python/custreamz/custreamz/kafka.py 29.16% <0.00%> (-0.63%) ⬇️
python/dask_cudf/dask_cudf/sorting.py 92.66% <0.00%> (-0.25%) ⬇️
python/dask_cudf/dask_cudf/core.py 70.85% <0.00%> (-0.17%) ⬇️
python/cudf/cudf/__init__.py 0.00% <0.00%> (ø)
python/cudf/cudf/core/frame.py 0.00% <0.00%> (ø)
python/cudf/cudf/core/index.py 0.00% <0.00%> (ø)
python/cudf/cudf/io/parquet.py 0.00% <0.00%> (ø)
python/cudf/cudf/core/series.py 0.00% <0.00%> (ø)
python/cudf/cudf/utils/utils.py 0.00% <0.00%> (ø)
python/cudf/cudf/api/__init__.py 0.00% <0.00%> (ø)
... and 24 more

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 7ff5f12...a433e4a. Read the comment docs.

@@ -171,8 +171,9 @@ std::vector<metadata::stripe_source_mapping> aggregate_orc_metadata::select_stri

// Coalesce stripe info at the source file later since that makes downstream processing much
// easier in impl::read
for (const size_t& stripe_idx : user_specified_stripes[src_file_idx]) {
CUDF_EXPECTS(stripe_idx < per_file_metadata[src_file_idx].ff.stripes.size(),
for (const auto& stripe_idx : user_specified_stripes[src_file_idx]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just small nit:

Suggested change
for (const auto& stripe_idx : user_specified_stripes[src_file_idx]) {
for (const auto stripe_idx : user_specified_stripes[src_file_idx]) {

Copy link
Contributor

Choose a reason for hiding this comment

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

@ttnghia Can you explain why you're suggesting this change?

Copy link
Contributor

@ttnghia ttnghia Jan 19, 2022

Choose a reason for hiding this comment

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

Sorry @bdice for late reply (I didn't see this).
A reference is similar to pointer, which is 64 bits. If you use reference for 32 bits numbers, that's kind of inefficient. Although the compiler can optimize this and generate the same code (for references and without references), it is still logically inefficient.

@robertmaynard
Copy link
Contributor Author

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 04b79ac into rapidsai:branch-22.02 Jan 18, 2022
@robertmaynard robertmaynard deleted the bug/orc_dont_take_temp_by_ref branch January 18, 2022 20:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for Review Ready for review by team bug Something isn't working libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants