-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Add Doxygen Comments
section into the code style guide
#5694
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
base: rolling
Are you sure you want to change the base?
Conversation
Signed-off-by: Michael Orlov <[email protected]>
HTML artifacts: https://github.com/ros2/ros2_documentation/actions/runs/15433438345/artifacts/3255803613. To view the resulting site:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm.
- 1st of all, we did not have any preference for doxygen comments rule. and there are already inconsistencies in the core libraries. i think it is good thing that we have clear preference what policy should be applied if there is a question.
- this is the preference, not enforcement (we do not have static checker either). but i think the preference still prevails when there are conflicts or questions about doxygen comment style. this makes the preferred doxygen comment style gradually rolls out to the repositories in consistent.
the thing i am not sure is that which style is actually preferred?... ///
or /** */
💦 this PR already describes the rational reasons, but any objections???
source/The-ROS2-Project/Contributing/Code-Style-Language-Versions.rst
Outdated
Show resolved
Hide resolved
Co-authored-by: Tomoya Fujita <[email protected]> Signed-off-by: Michael Orlov <[email protected]> Signed-off-by: Michael Orlov <[email protected]>
cf8f817
to
12064f0
Compare
I actually disagree with the rational here given for the /// in favor of /** */ For me personally I like that /** is a distinct start marker and */ is a distinct end maker. For me, this makes it easier to spot the start and end of the comment section, and eases the process of browsing header files. The compact rational is also something I don't agree with. I find it easier to read function comments, if they are not compact, but clearly structured. Empty line after the brief, and an empty line after the detailed description before the parameters are ideal from my point of view, as I can quickly jump to the section I am interested in. Typing inside of /** */ is also easier for me, as my editor knows that it is inside of a comment and auto adds ' * ' if I press enter. I guess this is highly subjective (and controversial). |
Since we diverge in opinions, it seems we need a broader discussion and more opinions on that matter. I personally found that our header files are cluttered with a lot of "dummy" lines Again As regards:
I have quite the opposite experience. Typing and editing, especially multiline comments inside of Overall, I found that dealing with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think some of the explanations/rationales given here are a bit subjective. However, overall, I'm in favour of picking 1 style and making it our explicit preference, leaving no ambiguity.
One downside to the mixed style:
/// Brief.
/**
* Detailed.
*
* \param name the name
* \return a value
*/
is that VS Code doesn't seem to parse it correctly. When hovering over a function, it only shows the /** ... */
part (detailed description, params, return, exceptions, etc.), at least with the default "C/C++" extension.
* rationale: ``///`` is more consistent with the rest of the C++ code. | ||
* rationale: ``///`` is easier to type and read in most editors. | ||
* rationale: The Doxygen comments with ``///`` is easier to read in the code itself, as it is | ||
more compact versus ``/** */`` which takes up more space and requires more lines. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to what @jmachowinski said (#5694 (comment)), I think these are highly subjective, especially numbers 2 and 3. I personally do not agree with them; I think /** */
is easier to type and easier to parse visually.
I'm Just trying to bring another perspective. I prefer picking a single style over picking a particular style.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my explanation in the #5694 (comment) about why it is not easy to type.
* While it is allowed to use ``/** */`` style comments for documentation, we prefer ``///`` style | ||
comments for Doxygen. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this wording is confusing, because it implies that people only use one or the other, while they instead might want to use a combination of both:
/// Brief.
/**
* Detailed.
*
* \param name the name
* \return a value
*/
So this should be clarified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it is implied to use one or another.
I clarified it by
- Providing an example when mixing styles is not good
- Adding notes with clarification right after rationale list
- Note that using
/** */
Doxygen comments is useful in cases when C++ header files could
participate in compilation with a C compiler, for instance, in C++/C mixed packages,
when exposed API is written to comply with the C standard.
- Need to keep consistency and adhere to the one style of Doxygen comments at least in the scope of
the one file, i.e. either use///
or/** */
style comments.
/** | ||
* Detailed description of the function. | ||
* \param param1 Description of the first parameter. | ||
* \param param2 Description of the second parameter. | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/** | |
* Detailed description of the function. | |
* \param param1 Description of the first parameter. | |
* \param param2 Description of the second parameter. | |
*/ | |
/** | |
* Detailed description of the function. | |
* \param param1 Description of the first parameter. | |
* \param param2 Description of the second parameter. | |
*/ |
Basically all IDEs take care of this automatically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As regards
Basically all IDEs take care of this automatically.
Actually not. At least I would say most IDEs are partially taking care of those prefixes automatically.
Most IDEs add a prefixing space before the *
marker when the user presses the Enter key.
However, when one tries to copy-paste such a block very often, that extra padding goes away.
And this is precisely what happened here! Believe me or not, it wasn't on purpose in this PR.
This is one of the major reasons why I am advocating for the C++ ///
style.
cc: @jmachowinski @tfoote
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sort of change that has large cross cutting ramifications should be approached more formally than with a slack conversation and a few people providing in put on a documentation PR.
Flip flopping on this and changing our standards will result in an even more chaotic codebase. If we want to make a change to these standards we should do so as a committee with everyone's full buy in. And as well we should look at planning a migration (be it incremental or wholesale) And potentially evaluating if we can add linters etc to enforce it. But particularly saying that what is commonly used right now will potentially cause a lot of potentially unnecessary churn and questions of do you follow the standard in this file or the new one.
I'm marking this as changes requested not specifically as I'm against the changes but to indicate that we should have a more complete process about this before we land this.
Description
This PR adds the
Doxygen Comments
section to the code style guide, i.e.,Code-Style-Language-Versions.rst
, with a description of the preferred style for the Doxygen comments.This PR is a follow-up on the discussion from the ros2/rclcpp#2858 (comment)
typesupport_helpers
API needed for the Rosbag2 rclcpp#2858Did you use Generative AI?
Yes, I am using GitHub Copilot v4.1 in my workflow.
Additional Information
N/A