Skip to content

Correct unused parameters in the copying algorithms - #8232

Merged
rapids-bot[bot] merged 7 commits into
NVIDIA:branch-21.06from
robertmaynard:bug/correct_unused_parameters_in_copying
May 19, 2021
Merged

Correct unused parameters in the copying algorithms#8232
rapids-bot[bot] merged 7 commits into
NVIDIA:branch-21.06from
robertmaynard:bug/correct_unused_parameters_in_copying

Conversation

@robertmaynard

@robertmaynard robertmaynard commented May 13, 2021

Copy link
Copy Markdown
Contributor

Starting in CUDA 11.3, nvcc will start to unconditionally warn about unused parameters on functions/methods that are in anonymous namespaces.

@robertmaynard
robertmaynard requested a review from a team as a code owner May 13, 2021 13:14
@github-actions github-actions Bot added the libcudf Affects libcudf (C++/CUDA) code. label May 13, 2021
davidwendt
davidwendt reviewed May 13, 2021
View reviewed changes
Comment thread cpp/src/copying/copy_range.cu Outdated
@robertmaynard robertmaynard changed the title correct unused parameters in copying algorithms [Review] Correct unused parameters in copying algorithms May 13, 2021
davidwendt
davidwendt reviewed May 14, 2021
View reviewed changes
Comment thread cpp/src/copying/segmented_shift.cu Outdated
@codecov

codecov Bot commented May 14, 2021 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #8232 (b16b214) into branch-0.20 (51336df) will increase coverage by 0.00%.
The diff coverage is n/a.

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

@@              Coverage Diff              @@
##           branch-0.20    #8232    +/-   ##
=============================================
  Coverage        82.88%   82.89%            
=============================================
  Files              103      105     +2     
  Lines            17668    17938   +270     
=============================================
+ Hits             14645    14870   +225     
- Misses            3023     3068    +45
Impacted Files Coverage Δ
python/cudf/cudf/core/tools/datetimes.py 80.42% <0.00%> (-4.11%) ⬇️
python/cudf/cudf/core/column/decimal.py 90.47% <0.00%> (-2.46%) ⬇️
python/cudf/cudf/core/column/datetime.py 88.03% <0.00%> (-1.88%) ⬇️
python/cudf/cudf/core/column/struct.py 94.73% <0.00%> (-1.56%) ⬇️
python/cudf/cudf/utils/dtypes.py 82.20% <0.00%> (-1.24%) ⬇️
python/dask_cudf/dask_cudf/groupby.py 91.28% <0.00%> (-0.88%) ⬇️
python/cudf/cudf/core/series.py 91.17% <0.00%> (-0.56%) ⬇️
python/cudf/cudf/core/index.py 92.52% <0.00%> (-0.55%) ⬇️
python/cudf/cudf/core/column/column.py 88.20% <0.00%> (-0.44%) ⬇️
python/cudf/cudf/core/column/lists.py 87.09% <0.00%> (-0.32%) ⬇️
... and 30 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 99df69f...f1adb67. Read the comment docs.

@robertmaynard robertmaynard added code quality non-breaking Non-breaking change 3 - Ready for Review Ready for review by team labels May 17, 2021
@karthikeyann

karthikeyann commented May 17, 2021

Copy link
Copy Markdown
Contributor

does this change affect compile time noticeably?

@robertmaynard

robertmaynard commented May 17, 2021

Copy link
Copy Markdown
Contributor Author

does this change affect compile time noticeably?

I checked just copy_range and contiguous_split as they had an increase in the amount of SFINAE.

copy_range compile time increased by 2% ( an increase of 0.68sec )
contiguous_split compile time went down by 1% ( a decrease of 0.14sec )

Overall I would say no major change in compile time.

@robertmaynard robertmaynard added the improvement Improvement / enhancement to an existing function label May 17, 2021
@robertmaynard robertmaynard changed the title [Review] Correct unused parameters in copying algorithms [Review] Correct unused parameters in the copying algorithms May 17, 2021
@harrism harrism changed the title [Review] Correct unused parameters in the copying algorithms Correct unused parameters in the copying algorithms May 18, 2021
harrism
harrism approved these changes May 18, 2021
View reviewed changes

@harrism harrism left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks fine, although [[maybe_unused]] may be a bit cleaner than (void)parameter;.

Comment thread cpp/src/copying/copy.cu
@robertmaynard

robertmaynard commented May 19, 2021

Copy link
Copy Markdown
Contributor Author

@gpucibot merge

@rapids-bot
rapids-bot Bot merged commit 6b09253 into NVIDIA:branch-21.06 May 19, 2021
@robertmaynard robertmaynard mentioned this pull request May 19, 2021
Merged
rapids-bot Bot pushed a commit that referenced this pull request May 19, 2021
@robertmaynard
Remove unused parameter from copy_partition kernel documentation (#8283)
5994751 
PR #8232 removed the `num_partitions` parameter from the `copy_partition` kernel but failed to update the associated documentation.

Authors:
  - Robert Maynard (https://github.com/robertmaynard)

Approvers:
  - Karthikeyan (https://github.com/karthikeyann)
  - David Wendt (https://github.com/davidwendt)

URL: #8283
@robertmaynard
robertmaynard deleted the bug/correct_unused_parameters_in_copying branch May 21, 2021 18:11
@robertmaynard robertmaynard mentioned this pull request May 28, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@davidwendt davidwendt davidwendt approved these changes
+1 more reviewer
@harrism harrism harrism approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

3 - Ready for Review Ready for review by team improvement Improvement / enhancement to an existing function libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@robertmaynard @karthikeyann @harrism @davidwendt