Fix Doxygen @param and @tparam names that do not match the declarations - #23564
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis change corrects Doxygen template parameter names and annotations across cuDF column, vector factory, utility, rolling, string, and test headers. It also updates selected SPDX notices. Function signatures and runtime behavior remain unchanged. ChangesDoxygen documentation corrections
Estimated code review effort: 1 (Trivial) | ~3 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
The Label Checker is red because I can't add labels from a fork — this needs |
igorpeshansky
left a comment
There was a problem hiding this comment.
Three more include files with the same issues: cpp/include/cudf/column/column_device_view_base.cuh (#23564 (comment)), cpp/include/cudf/detail/iterator.cuh (#23564 (comment)), and cpp/include/cudf_test/timestamp_utilities.cuh (#23564 (comment)).
[Really optional] Also in cpp/src: cpp/src/rolling/detail/range_utils.cuh:570 and cpp/src/strings/count_matches.hpp:32.
| */ | ||
| bool is_bit_castable(data_type from, data_type to); | ||
|
|
||
| template <typename From, typename To> |
There was a problem hiding this comment.
[Optional] Prior code, but undocumented… Seems like this PR is a good place to add a doc for cudf::is_convertible, but feel free to defer to a follow-up.
| * | ||
| * @tparam KeyElementTo Specify a fixed-width type for the key values of the dictionary | ||
| * @tparam SourceElementTo For converting fixed-width values to the KeyElementTo | ||
| * @tparam SourceElementT For converting fixed-width values to the KeyElementTo |
There was a problem hiding this comment.
| * @tparam SourceElementT For converting fixed-width values to the KeyElementTo | |
| * @tparam SourceElementT For converting fixed-width values to the `KeyElementTo` |
[Optional] fixed_width_column_wrapper uses the following phrasing:
@tparam SourceElementT The fixed-width element type that is used to create elements of type `ElementTo`
Worth making them consistent (one way or another)?
There was a problem hiding this comment.
Quick ping on the second (optional) half of that comment… Would love to hear your thoughts.
There was a problem hiding this comment.
Sorry, that half slipped past me. Took the fixed_width_column_wrapper wording since the dictionary wrapper is doing the same job, so the two read the same way now.
- column_device_view_base.cuh: name the head<T>() template parameter - type_dispatcher.hpp: the parameter is IdTypeMap, id_to_type_impl is its default - iterator.cuh, range_utils.cuh, count_matches.hpp: name the template parameter - timestamp_utilities.cuh: Rep and Period are local aliases, the parameters are T and nullable - traits.hpp: the return descriptions refer to T, not to a data_type argument - column_wrapper.hpp: mark up KeyElementTo as code
| * | ||
| * @tparam KeyElementTo Specify a fixed-width type for the key values of the dictionary | ||
| * @tparam SourceElementTo For converting fixed-width values to the KeyElementTo | ||
| * @tparam SourceElementT For converting fixed-width values to the KeyElementTo |
There was a problem hiding this comment.
Quick ping on the second (optional) half of that comment… Would love to hear your thoughts.
|
/ok to test e3abeb9 |
| * trying to return different types from the same function. | ||
| * | ||
| * @tparam id_to_type_impl Maps a `cudf::type_id` its dispatched C++ type | ||
| * @tparam IdTypeMap Maps a `cudf::type_id` its dispatched C++ type |
There was a problem hiding this comment.
[Optional] Just noticed a (prior) typo:
| * @tparam IdTypeMap Maps a `cudf::type_id` its dispatched C++ type | |
| * @tparam IdTypeMap Maps a `cudf::type_id` to its dispatched C++ type |
There was a problem hiding this comment.
Applied, the to was missing
|
/ok to test 59f1cc1 |
Signed-off-by: Igor Peshansky <ipeshansky@nvidia.com>
|
/ok to test a0110b6 |
|
/ok to test 940b078 |
|
Two commits since your approval, both from the optional comments: the |
|
/ok to test 3e199ff |
|
/ok to test 137cbe5 |
|
/ok to test 44441ff |
|
/ok to test 0057962 |
|
/merge |
Description
Doxygen tags in
cpp/includeandcpp/srcthat name something the declaration below them does not have. Comments only — no code, no behaviour, no tests.@tparamwith the name missing —column_view_base::headandmutable_column_view::headboth carry@tparam The type to cast to, so Doxygen takesTheas the template parameter name andTgoes undocumented.@tparamleft over from a refactor — eight of themake_device_uvector*overloads indetail/utilities/vector_factories.hppdocument bothContainerandT, but those overloads aretemplate <typename Container>only. TheTline is a leftover from thehost_span<T>overloads above them.@tparamrenamed —id_to_typedocumentstwhile the parameter isId;dictionary_column_wrapperdocumentsSourceElementTowhile the parameter isSourceElementT.@paramon a function that takes nothing —arrow_column::view()andarrow_table::view()documentstreamandmr, copied from theto_arrowoverload directly above each. Both take no arguments.@paramwhere@tparamwas meant —is_boolean()documents@param type The data_type to verifyandis_nested()documents@param T; both aretemplate <typename T>predicates with no runtime arguments.A second commit covers the further files pointed out in review:
column_device_view_base.cuh,type_dispatcher.hpp'sIdTypeMap,iterator.cuh,timestamp_utilities.cuh,rolling/detail/range_utils.cuhandstrings/count_matches.hpp.Follows #21762 and #21764, which did the same for other entries in
/includeand/src. Every tag here was opened and read against the declaration underneath it.Checklist