Fix doxygen warnings #1317

vyasr · 2023-08-01T01:00:15Z

Description

This PR fixes all doxygen warnings from building the C++ documentation.

Checklist

I am familiar with the Contributing Guidelines.
New or existing tests cover these changes.
The documentation is up to date with these changes.

bdice

A handful of comments. Most are small but a couple are blocking so I'll request changes.

include/rmm/device_buffer.hpp

include/rmm/device_scalar.hpp

include/rmm/exec_policy.hpp

include/rmm/mr/device/fixed_size_memory_resource.hpp

include/rmm/mr/device/logging_resource_adaptor.hpp

include/rmm/mr/device/statistics_resource_adaptor.hpp

include/rmm/mr/device/thrust_allocator_adaptor.hpp

include/rmm/mr/host/pinned_memory_resource.hpp

include/rmm/cuda_device.hpp

include/rmm/device_buffer.hpp

include/rmm/device_scalar.hpp

include/rmm/device_uvector.hpp

harrism

I didn't make it all the way through. I definitely value fixing cases where things were undocumented that really should be, or that had // documents that don't show up in Doxygen. But there's a lot of irritating superfluous documentation in this PR and a lot of duplication (@brief == @return) that I would rather avoid.

Can the warnings be suppressed in other ways? Can we just live with or ignore some warnings?

include/rmm/cuda_device.hpp

include/rmm/cuda_stream_view.hpp

include/rmm/device_buffer.hpp

include/rmm/device_scalar.hpp

include/rmm/exec_policy.hpp

include/rmm/logger.hpp

vyasr · 2023-08-02T23:01:47Z

I've defined ALIASES for @brief + @return as requested, and also for copy/move constructors/assignment. For assignment operators it's helpful since it turns a 5 line docstring into a short one-liner. For the constructors I'm not convinced it's a good idea; it doesn't improve brevity or readability and it introduces some extra cognitive overload (a couple more commands). The only (very minor IMO) benefit is that it ensure that all those functions are documented identically across all classes. I'm curious to hear how others feel, but I'm inclined to remove those.

bdice · 2023-08-04T18:21:36Z

I'm curious to hear how others feel, but I'm inclined to remove those.

I don't feel strongly about how this is implemented, I might be weakly in favor of removing them and using "normal" Doxygen rather than aliases. I'd approve it either way though.

harrism · 2023-08-15T23:30:39Z

@vyasr have you seen doxygen Member Groups? I just ran across them. Might be useful for group-documenting defaulted ctors/methods, using declarations, etc.? https://www.doxygen.nl/manual/grouping.html#memgroup

vyasr · 2023-08-17T21:39:09Z

No, I haven't seen those before, only the module-level groups. That certainly could be helpful for improving the documentation.

…en_warnings

vyasr · 2023-08-28T16:28:12Z

I've addressed some of the requests made during review of this PR. In the interest of making what's left for this PR actionable, here are the outstanding issues that I see, along with potential unapplied mitigations. Then we can evaluate whether any of the issues with a solution are dealbreakers:

Documenting copy/move constructors/assignments, even if they are defined as default: these are required, but I've added an alias to at least reduce the duplication
device_scalar unused using declarations: I've left these docs in for now, but I think we could remove these aliases altogether in a follow-up PR because they are unused. I didn't want to do it in this PR because it would technically be a breaking change

Aside from that I think that aliases and some cleanup have resolved the worst remaining offenders from the first pass of changes.

@harrism @bdice @wence- WDYT?

harrism

Looking good. Some suggestions for type documentation and a couple of corrections.

include/rmm/cuda_device.hpp

include/rmm/device_buffer.hpp

include/rmm/device_scalar.hpp

include/rmm/mr/device/logging_resource_adaptor.hpp

include/rmm/mr/device/polymorphic_allocator.hpp

wence-

Some more minor issues, but @harrism covered most of them.

include/rmm/device_scalar.hpp

include/rmm/mr/device/thread_safe_resource_adaptor.hpp

Co-authored-by: Mark Harris <783069+harrism@users.noreply.github.com> Co-authored-by: Lawrence Mitchell <wence@gmx.li>

harrism

Looks good. Thanks for pushing it through!

All comments/issues addressed

vyasr · 2023-09-01T17:24:29Z

/merge

doxygen catches more doc issues (of the types fixed in #1317) when more build outputs are turned on, which is indicative of some bugs/limitations in doxygen. XML builds will be necessary to leverage Breathe (see #1324) so this PR enables XML builds and fixes the associated issues. Authors: - Vyas Ramasubramani (https://github.com/vyasr) Approvers: - Rong Ou (https://github.com/rongou) URL: #1348

This PR leverages [Breathe](https://breathe.readthedocs.io/en/latest/) to pull the rmm C++ API documentation into the python Sphinx docs build, generating a single unified build of the documentation that supports cross-linking between language libraries and also simplifies cross-linking from other libraries that wish to link here (such as higher-level RAPIDS libraries that use both rmm's Python and C++ APIs). Using Breathe requires changing the doxygen build to generate XML in addition to the usual HTML. It turns out that doxygen catches more doc issues (of the types fixed in #1317) when more build outputs are turned on, which is indicative of some bugs/limitations in doxygen, but nonetheless I've fixed the additional issues in this PR as well. Authors: - Vyas Ramasubramani (https://github.com/vyasr) Approvers: - Mark Harris (https://github.com/harrism) - Bradley Dice (https://github.com/bdice) - AJ Schmidt (https://github.com/ajschmidt8) URL: #1324

vyasr added 2 commits July 31, 2023 17:52

Fix most docs issues

171bd2d

Fix issue with doxygen not supporting unnamed parameters

6fb60db

vyasr added doc Documentation non-breaking Non-breaking change labels Aug 1, 2023

vyasr requested a review from a team as a code owner August 1, 2023 01:00

vyasr self-assigned this Aug 1, 2023

vyasr requested review from rongou and bdice August 1, 2023 01:00

github-actions bot added the cpp Pertains to C++ code label Aug 1, 2023

bdice previously requested changes Aug 1, 2023

View reviewed changes

wence- reviewed Aug 1, 2023

View reviewed changes

include/rmm/cuda_device.hpp Outdated Show resolved Hide resolved

include/rmm/device_buffer.hpp Outdated Show resolved Hide resolved

include/rmm/device_scalar.hpp Outdated Show resolved Hide resolved

include/rmm/device_uvector.hpp Show resolved Hide resolved

wence- mentioned this pull request Aug 1, 2023

Use size_type in device_uvector definitions when accepting/returning something of size_type #1318

Open

harrism reviewed Aug 1, 2023

View reviewed changes

vyasr added 6 commits August 1, 2023 11:13

Fix typo

2aec456

Fix some of the real issues in the docs

d9bc2b4

Remove docs of deleted functions

5b25a71

Add briefreturn alias for simplifying getters

2e46ec3

Fix typo

3227773

Use aliases for copy/move constructors/assignment operators

d0ecdaa

vyasr mentioned this pull request Aug 17, 2023

Generate unified Python/C++ docs #1324

Merged

3 tasks

vyasr added 2 commits August 28, 2023 08:55

Merge remote-tracking branch 'origin/branch-23.10' into doc/fix_doxyg…

178691e

…en_warnings

Address remaining reviews

7d5c80d

vyasr requested review from harrism, wence- and bdice August 29, 2023 03:57

harrism requested changes Aug 29, 2023

View reviewed changes

wence- reviewed Aug 29, 2023

View reviewed changes

include/rmm/device_scalar.hpp Outdated Show resolved Hide resolved

include/rmm/mr/device/thread_safe_resource_adaptor.hpp Show resolved Hide resolved

vyasr and others added 3 commits August 30, 2023 14:47

Apply suggestions from code review

1229220

Co-authored-by: Mark Harris <783069+harrism@users.noreply.github.com> Co-authored-by: Lawrence Mitchell <wence@gmx.li>

Remaining PR comments

6f105f5

Fix all periods

c942202

vyasr mentioned this pull request Aug 30, 2023

[FEA] Consider making the lock types of memory resources private #1331

Open

vyasr requested review from wence- and harrism August 30, 2023 22:19

harrism approved these changes Aug 31, 2023

View reviewed changes

rongou approved these changes Aug 31, 2023

View reviewed changes

wence- approved these changes Sep 1, 2023

View reviewed changes

rapids-bot bot merged commit 93e616a into rapidsai:branch-23.10 Sep 1, 2023
44 checks passed

vyasr deleted the doc/fix_doxygen_warnings branch September 1, 2023 17:24

vyasr mentioned this pull request Sep 25, 2023

Enable doxygen XML and fix issues #1348

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix doxygen warnings #1317

Fix doxygen warnings #1317

vyasr commented Aug 1, 2023

bdice left a comment

harrism left a comment •

edited

Loading

vyasr commented Aug 2, 2023

bdice commented Aug 4, 2023

harrism commented Aug 15, 2023

vyasr commented Aug 17, 2023

vyasr commented Aug 28, 2023

harrism left a comment

wence- left a comment

harrism left a comment

vyasr commented Sep 1, 2023

Fix doxygen warnings #1317

Fix doxygen warnings #1317

Conversation

vyasr commented Aug 1, 2023

Description

Checklist

bdice left a comment

Choose a reason for hiding this comment

harrism left a comment • edited Loading

Choose a reason for hiding this comment

vyasr commented Aug 2, 2023

bdice commented Aug 4, 2023

harrism commented Aug 15, 2023

vyasr commented Aug 17, 2023

vyasr commented Aug 28, 2023

harrism left a comment

Choose a reason for hiding this comment

wence- left a comment

Choose a reason for hiding this comment

harrism left a comment

Choose a reason for hiding this comment

vyasr commented Sep 1, 2023

harrism left a comment •

edited

Loading