Refactor tests/iterator_utilities.hpp functions
#8540
Conversation
ttnghia
added
feature request
Codecov Report
@@ Coverage Diff @@
## branch-21.08 #8540 +/- ##
===============================================
Coverage ? 82.96%
===============================================
Files ? 109
Lines ? 18197
Branches ? 0
===============================================
Hits ? 15097
Misses ? 3100
Partials ? 0 Continue to review full report at Codecov.
|
| @@ -77,9 +75,9 @@ template <typename Iter> | |||
| * @param indices The indices for which the validity iterator must return `false` (i.e. null) | |||
| * @return auto Validity iterator | |||
| */ | |||
| [[maybe_unused]] static auto iterator_with_null_at(cudf::host_span<cudf::size_type const> indices) | |||
| [[maybe_unused]] static auto nulls_at(std::vector<cudf::size_type> const& indices) | |||
There was a problem hiding this comment.
When I first wrote it, this API used to take a std::vector. The suggestion at the time was that host_spans are preferable.
I agree with your current change. (Personally, I prefer this be vector<> so that call sites don't have to explicitly construct vectors.) I hope this is agreeable with @harrism and/or @vuule.
There was a problem hiding this comment.
Yeah, with host_span parameter, you cannot call nulls_at({1, 2, 3}). You must call nulls_at(std::vector<int>{1, 2, 3}). This is so much inconvenient.
|
Ah, I missed that @harrism has already approved this PR. I think we might be good to go. |
|
@gpucibot merge |
This PR refactors the functions in
tests/iterator_utilities.hpp. In particular:iteratorsiterator_with_from the function names. As such, instead of havingcudf::test::iterators_with_null_at(and similarcudf::test::iterators_with_*), we will havecudf::test::iterators::null_at(and similar). Upon usage, we can import the namespacecudf::test::iteratorsand use justnull_at().This refactor will make the test code look significantly cleaner. The subnamespace
iteratorsmakes sure that there will be no name conflict.