-
Notifications
You must be signed in to change notification settings - Fork 163
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
Change naming style for private functions #597
Merged
Merged
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
What does private function mean in
C
?This function should be
static
, there's no naming convention needed.According to the C++ standard, names starting with
_
or names containing__
are reserved to the implementation. See https://wiki.sei.cmu.edu/confluence/display/cplusplus/DCL51-CPP.+Do+not+declare+or+define+a+reserved+identifier#DCL51-CPP.I would rather delete leading underscores that add more.
We could use another convention for static functions, like
s_
, to make the name more obvious.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 old related discussion: ros2/rclcpp#874
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 agree with you that by using
static
is enough so that the function is used only in the module.I saw other functions with the leading underscore and wanted them to use the same naming convention. I can change this PR to remove the underscore and use the static keyword in all in the functions internal to the module.
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.
Having a naming convention is good but using a leading underscore is not standard compliant, so I would avoid that particular convention.
For the moment, I would just add
static
where missing. We can iterate on a correct naming convention later, maybe usings_
or maybe just using no prefix (i.e.: deleting current leading underscores).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.
Should I remove the leading underscore for the functions that already had it before my commit?
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.
"Private" in this case is indicating that it's not a public API intended to be stable for external reuse: https://index.ros.org/doc/ros2/Contributing/Developer-Guide/#public-api-declaration It's about documentation more than enforcement.
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.
The function in question can't be used externally anyways since it's not declared in a header.
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, the functions can't be used externally so it shouldn't be a problem related to API. I opened the issue because it's odd browsing through the code and not having some uniformity regarding names. e.g:
rcl_init_generic_clock
and_rcl_clock_generic_fini
. I'll proceed removing the underscores and adding the static keyword instead.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.
IIRC, that's not 100% true. If the function isn't marked as
static
, you can declare it in another executable without definition, then link again this library and use it (that probably doesn't apply to Windows DLLs, because of their export/import stuff).Functions not declared in a library header shouldn't be considered part of the API though.
👍
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.
That's right, thanks for the clarification. Hopefully no one is adding a declaration anywhere.