Fix Doxygen warnings in table header files #10964

karthikeyann · 2022-05-25T12:07:31Z

Fixes parts of #9373
added missing documentation to fix doxygen warnings in table headers
ignores doc generation for strong_index_comparator_adapter

fixes 166 warnings.

cpp/include/cudf/table/experimental/row_operators.cuh

davidwendt · 2022-05-25T13:33:24Z

cpp/include/cudf/table/row_operators.cuh

- * @param lhs first element
- * @param rhs second element
- * @return Indicates the relationship between the elements in
- * the `lhs` and `rhs` columns.


Seems this was accidentally removed?

No. doxygen throws warning that params are documented twice because of SFINAE. These arguments are documented in other definition.
warning: argument 'lhs' from the argument list of cudf::relational_compare has multiple @param documentation sections
so, this params doc is removed.

Both @brief will show in the documentation. https://docs.rapids.ai/api/libcudf/nightly/namespacecudf.html#a9b480332cf7484e8532a2d6f2c711a35

Hmm that seems problematic. Given our extensive use of SFINAE, we should probably standardize how we choose to document that. Since as @davidwendt mentioned that most readers of our docstrings are probably developers. It's especially odd here because the null_compare is in between the two overloads, so as a developer I wouldn't immediately know where to look for the docs. I would say for consistency that it's more important that all SFINAE overloads are documented, so maybe we need to tell doxygen to ignore certain warnings? Alternatively, if we could guarantee that all SFINAE overloads were next to each other we could standardize on documenting all overloads on just the first instance, but that's hard to enforce.

I don't know how anyone else feels, I'm sure that some people like @harrism would have opinions as well.

So, honestly, any functions that requires SFINAE should probably not be in a public header. That is, I cannot picture a non-internal libcudf API that would have SFINAE semantics. And there has already been discussion on moving the row-operators into the detail namespace. In general, I would not spend too much time on trying to get doxygen working with SFINAE'd functions.

That is a fair point. I guess the question then would be, should doxygen be linting those at all? It's a bit tricky right now because we also use detail to hold the stream versions of public APIs, and those may eventually become public, but in the longer run we probably want to tell doxygen either not to lint those files or to at least relax some requirements.

Anyway, I'm fine with this PR not resolving that question.

cpp/include/cudf/table/experimental/row_operators.cuh

vyasr · 2022-05-25T18:34:13Z

cpp/include/cudf/table/experimental/row_operators.cuh

@@ -899,6 +928,10 @@ struct preprocessed_table {
  std::vector<rmm::device_buffer> _null_buffers;
 };

+/**
+ * @brief Comparator for performing equality comparisons between two rows of the same table.
+ *


Does doxygen style require this extra line after the brief even when there are no additional lines afterwards, or is that something that you are doing? It seems like an unnecessary extra line to me, but maybe it's just what we need to do?

doxygen does not require it. The vscode doxygen extension inserts this line by default.

I see. I think I saw a few other places where there were param/return docs after the brief, and there was no space in between. It would be nice to make those consistent, but if doxygen doesn't require it then I wouldn't waste time on that (except when adding new lines) until after these PRs are all set.

AFAIK, new lines are for readability. I would prefer to leave it as the doxygen extension generates.
it does not affect generated docs.

I will create a single PR with other changes like removing dots at end of @param, order of https://github.com/rapidsai/cudf/blob/branch-22.06/cpp/docs/DOCUMENTATION.md#function-parameters , //!< to ///<.

do you suggest to add "compulsory newline after @brief" in that PR?

Personally yes, I like the newline after brief. Not only is it easier to read, it makes it very obvious when the brief is not actually very brief if there's an extremely long line.

cpp/include/cudf/table/experimental/row_operators.cuh

cpp/include/cudf/table/row_operators.cuh

Co-authored-by: Vyas Ramasubramani <[email protected]> Co-authored-by: David Wendt <[email protected]>

codecov · 2022-05-28T02:10:31Z

Codecov Report

❗ No coverage uploaded for pull request base (branch-22.08@5367858). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head 3cedfb6 differs from pull request most recent head 7e2fe00. Consider uploading reports for the commit 7e2fe00 to get more accurate results

@@               Coverage Diff               @@
##             branch-22.08   #10964   +/-   ##
===============================================
  Coverage                ?   86.32%           
===============================================
  Files                   ?      144           
  Lines                   ?    22696           
  Branches                ?        0           
===============================================
  Hits                    ?    19593           
  Misses                  ?     3103           
  Partials                ?        0

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 5367858...7e2fe00. Read the comment docs.

cpp/include/cudf/table/table_view.hpp

cpp/include/cudf/table/table_device_view.cuh

vyasr

I'm mostly happy, but would like the discussion about SFINAE overloads resolved before I approve.

vyasr · 2022-05-31T19:59:00Z

cpp/include/cudf/table/experimental/row_operators.cuh

@@ -899,6 +928,10 @@ struct preprocessed_table {
  std::vector<rmm::device_buffer> _null_buffers;
 };

+/**
+ * @brief Comparator for performing equality comparisons between two rows of the same table.
+ *


Personally yes, I like the newline after brief. Not only is it easier to read, it makes it very obvious when the brief is not actually very brief if there's an extremely long line.

cpp/include/cudf/table/experimental/row_operators.cuh

vyasr · 2022-05-31T20:10:02Z

cpp/include/cudf/table/row_operators.cuh

- * @param lhs first element
- * @param rhs second element
- * @return Indicates the relationship between the elements in
- * the `lhs` and `rhs` columns.


Hmm that seems problematic. Given our extensive use of SFINAE, we should probably standardize how we choose to document that. Since as @davidwendt mentioned that most readers of our docstrings are probably developers. It's especially odd here because the null_compare is in between the two overloads, so as a developer I wouldn't immediately know where to look for the docs. I would say for consistency that it's more important that all SFINAE overloads are documented, so maybe we need to tell doxygen to ignore certain warnings? Alternatively, if we could guarantee that all SFINAE overloads were next to each other we could standardize on documenting all overloads on just the first instance, but that's hard to enforce.

I don't know how anyone else feels, I'm sure that some people like @harrism would have opinions as well.

Co-authored-by: David Wendt <[email protected]>

…doc-fix_warnings15

cpp/include/cudf/table/table_view.hpp

Co-authored-by: David Wendt <[email protected]>

cpp/include/cudf/table/table_view.hpp

Co-authored-by: David Wendt <[email protected]>

karthikeyann · 2022-06-02T01:09:52Z

@gpucibot merge

add missing docs in table headers

202c2f2

karthikeyann added 3 - Ready for Review Ready for review by team doc Documentation libcudf Affects libcudf (C++/CUDA) code. 4 - Needs Review Waiting for reviewer to review or respond non-breaking Non-breaking change labels May 25, 2022

karthikeyann requested a review from a team as a code owner May 25, 2022 12:07

karthikeyann self-assigned this May 25, 2022

karthikeyann requested review from devavret and davidwendt May 25, 2022 12:07

davidwendt requested changes May 25, 2022

View reviewed changes

vyasr requested changes May 25, 2022

View reviewed changes

karthikeyann and others added 2 commits May 28, 2022 05:13

Apply suggestions from code review

e8abed0

Co-authored-by: Vyas Ramasubramani <[email protected]> Co-authored-by: David Wendt <[email protected]>

address review comments

5840fa9

karthikeyann requested review from vyasr and davidwendt May 28, 2022 00:10

karthikeyann mentioned this pull request May 28, 2022

Fix doxygen warnings in structs, lists headers #10923

Merged

davidwendt reviewed May 31, 2022

View reviewed changes

cpp/include/cudf/table/table_view.hpp Outdated Show resolved Hide resolved

davidwendt reviewed May 31, 2022

View reviewed changes

cpp/include/cudf/table/table_device_view.cuh Outdated Show resolved Hide resolved

vyasr reviewed May 31, 2022

View reviewed changes

Apply suggestions from code review

41f1e8c

Co-authored-by: David Wendt <[email protected]>

karthikeyann requested review from vyasr and davidwendt June 1, 2022 12:44

karthikeyann added 2 commits June 1, 2022 18:29

newline style fix

d7b715a

Merge branch 'branch-22.08' of https://github.com/rapidsai/cudf into …

cbab55c

…doc-fix_warnings15

davidwendt reviewed Jun 1, 2022

View reviewed changes

cpp/include/cudf/table/table_view.hpp Outdated Show resolved Hide resolved

Update cpp/include/cudf/table/table_view.hpp

ffbb181

Co-authored-by: David Wendt <[email protected]>

karthikeyann requested a review from davidwendt June 1, 2022 14:42

davidwendt reviewed Jun 1, 2022

View reviewed changes

cpp/include/cudf/table/table_view.hpp Show resolved Hide resolved

Update cpp/include/cudf/table/table_view.hpp

7e2fe00

Co-authored-by: David Wendt <[email protected]>

karthikeyann requested a review from davidwendt June 1, 2022 14:58

davidwendt approved these changes Jun 1, 2022

View reviewed changes

vyasr approved these changes Jun 1, 2022

View reviewed changes

rapids-bot bot merged commit c01a2a4 into rapidsai:branch-22.08 Jun 2, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Doxygen warnings in table header files #10964

Fix Doxygen warnings in table header files #10964

karthikeyann commented May 25, 2022

davidwendt May 25, 2022

karthikeyann May 28, 2022 •

edited

Loading

vyasr May 31, 2022

davidwendt May 31, 2022

vyasr Jun 1, 2022

vyasr May 25, 2022

karthikeyann May 25, 2022

vyasr May 26, 2022

karthikeyann May 28, 2022 •

edited

Loading

vyasr May 31, 2022

codecov bot commented May 28, 2022 •

edited

Loading

vyasr left a comment

vyasr May 31, 2022

vyasr May 31, 2022

karthikeyann commented Jun 2, 2022

Fix Doxygen warnings in table header files #10964

Fix Doxygen warnings in table header files #10964

Conversation

karthikeyann commented May 25, 2022

Choose a reason for hiding this comment

karthikeyann May 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karthikeyann May 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented May 28, 2022 • edited Loading

Codecov Report

vyasr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karthikeyann commented Jun 2, 2022

karthikeyann May 28, 2022 •

edited

Loading

karthikeyann May 28, 2022 •

edited

Loading

codecov bot commented May 28, 2022 •

edited

Loading