-
Notifications
You must be signed in to change notification settings - Fork 908
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
Move lists utility function definition out of header #7266
Move lists utility function definition out of header #7266
Conversation
Codecov Report
@@ Coverage Diff @@
## branch-0.18 #7266 +/- ##
===============================================
+ Coverage 82.09% 82.20% +0.11%
===============================================
Files 97 100 +3
Lines 16474 16952 +478
===============================================
+ Hits 13524 13936 +412
- Misses 2950 3016 +66
Continue to review full report at Codecov.
|
Just an observation. Neither the |
@davidwendt, I've switched these to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replacing new code with existing, identical code. I like your style!
Letting @vuule, @karthikeyann, and @rapidsai/ops-codeowners off the hook on this one. Thanks for the reviews, @davidwendt, @nvdbaranec, @hyperbolic2346! |
rerun tests |
rerun tests |
@kkraus14, could this PR please be merged? I'm afraid the |
@gpucibot merge |
@kkraus14, Thank you! Wow, that was quick. |
Fixes #7265.
cudf::detail::get_num_child_rows()
is currently defined incudf/lists/detail/utilities.cuh
. The build pipelines for #7189 are fine, but there seem to be build failures in dependent projects such asspark-rapids
:In any case, it is less than ideal for the function to be completely defined in the header, especially given that the likes of
hashing.cu
are exposed to it (by way ofscatter.cuh
).This commit moves the function definition to a separate translation unit, without changing implementation or interface.