Skip to content
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

Fix Doxygen warnings in multiple headers files #11003

Merged
merged 24 commits into from
Jun 6, 2022

Conversation

karthikeyann
Copy link
Contributor

@karthikeyann karthikeyann commented May 27, 2022

Fixes parts of #9373
added missing documentation to fix doxygen warnings in multiple headers
cudf_test/, nvtext/, cudf_kafka/, ipc.hpp

With this PR, there are zero doxygen warnings! ☺️

@karthikeyann 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 27, 2022
@karthikeyann karthikeyann requested a review from a team as a code owner May 27, 2022 23:52
@karthikeyann karthikeyann self-assigned this May 27, 2022
@codecov
Copy link

codecov bot commented May 28, 2022

Codecov Report

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

❗ Current head 21568f0 differs from pull request most recent head 4fb276b. Consider uploading reports for the commit 4fb276b to get more accurate results

@@               Coverage Diff               @@
##             branch-22.08   #11003   +/-   ##
===============================================
  Coverage                ?   86.33%           
===============================================
  Files                   ?      144           
  Lines                   ?    22706           
  Branches                ?        0           
===============================================
  Hits                    ?    19604           
  Misses                  ?     3102           
  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 840fc87...4fb276b. Read the comment docs.

@davidwendt
Copy link
Contributor

I would consider not including cudf_test in our doxygen actually.

Comment on lines 97 to 105
/// Are the types same - simply copy elements from [begin, end) to out
/**
* @brief No conversion necessary
*
* @tparam FromT Source type
* @tparam ToT Target type
* @param element Source value
* @return The converted target value, same as source value
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Why don't just combine this?

* `cudf::testing::Types`. This is a drop in replacement for GTest's
* `::testing::Types`. In lieu of including `gtest/gtest.h`, include
* @code ::testing::Types @endcode. In lieu of including `gtest/gtest.h`, include
Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer the hashmarks for inline formatting. Did this fix a warning?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Even though it linked correctly with ` in the html page, doxygen still gave a warning that it cannot find this symbol. This change fixed the warning.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why using @code tag here for reference? I thought that @ref is more suitable here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since cudf_gtest.hpp is excluded, reference to ::testing::Types is removed.

@karthikeyann
Copy link
Contributor Author

rerun tests

@karthikeyann
Copy link
Contributor Author

Thank you @davidwendt @PointKernel @ttnghia

@karthikeyann
Copy link
Contributor Author

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 4d7595f into rapidsai:branch-22.08 Jun 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for Review Ready for review by team 4 - Needs Review Waiting for reviewer to review or respond doc Documentation libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants