Add new clang-tidy style rules

[sjahr]

Description

As discussed in our previous standup some additional style guidelines. I've added new rules for const member variables and functions in general + an additional check for static variables in anonymous namespaces. However, I did not come up with a good solution to the problem that initiated this discussion: static member variables.

According to the documentation there are rules for the StaticConstCase and the StaticVariableCase but not specifically for the static member case (right now they are subject to the private/protected member naming conventions). The ROS cpp style guide even discourages using static members in the first place.

Should we try to remove avoid static class members rather than adding an explicit naming convention for them and do we want to introduce a global naming convention for static variables?

Furthermore, the introduced naming rule for free functions diverges from the ROS style guidelines. Do we want that or should this rule be changed to camelBack?

Checklist

• Required by CI: Code is auto formatted using clang-format
• Extend the tutorials / documentation reference
• Document API changes relevant to the user in the MIGRATION.md notes
• Create tests, which fail without this PR reference
• Include a screenshot if changing a GUI
• While waiting for someone to review your request, please help review another open pull request to support the maintainers

[codecov]

Codecov Report

Attention: 68 lines in your changes are missing coverage. Please review.

Comparison is base (7fba8c6) 50.78% compared to head (f85727b) 50.77%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2177      +/-   ##
==========================================
- Coverage   50.78%   50.77%   -0.01%     
==========================================
  Files         392      392              
  Lines       32125    32125              
==========================================
- Hits        16313    16307       -6     
- Misses      15812    15818       +6     

Files	Coverage Δ
...e/collision_detection_fcl/src/collision_common.cpp	73.08% <100.00%> (ø)
...kinematic_constraints/src/kinematic_constraint.cpp	74.65% <100.00%> (ø)
...core/planning_interface/src/planning_interface.cpp	60.00% <100.00%> (ø)
...g_request_adapter/src/planning_request_adapter.cpp	78.85% <100.00%> (ø)
moveit_core/robot_model/src/joint_model_group.cpp	65.52% <ø> (ø)
moveit_core/robot_model/src/robot_model.cpp	74.88% <100.00%> (ø)
moveit_core/robot_state/src/conversions.cpp	63.71% <100.00%> (ø)
...include/moveit/robot_trajectory/robot_trajectory.h	98.67% <ø> (ø)
...eit_core/robot_trajectory/src/robot_trajectory.cpp	66.86% <100.00%> (ø)
...cessing/src/time_optimal_trajectory_generation.cpp	86.37% <ø> (ø)
... and 37 more

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[ChrisThrasher]

readability-static-definition-in-anonymous-namespace is a good check

[rhaschke]

The ROS cpp style guide discourages using static members.

Discouraging doesn't mean that static members are not allowed meaningful.
If you do want to share information across multiple instances of a class, a static member is the way to go. For sure, you then need to handle synchronization issues in multi-threaded environments.

Should we try to remove static class members rather than adding an explicit naming convention for them and do we want to introduce a global naming convention for static variables?

I strongly advise not to do this! As far as I know, MoveIt only sparsely uses static members - if they are needed.
Most usages are const static, which don't pose sync issues anyway.

Furthermore, the introduced naming rule for free functions diverges from the ROS style guidelines. Do we want that or should this rule be changed to camelBack?

Why, in first place, you want to change free functions to lower_case style? This will invalidate all existing free functions, thus also breaking API.

Note, that clang-tidy runs on code changes only by default. Thus changes to the .clang-tidy config file do not automatically trigger a check of the whole code base w.r.t. these new rules.

[sjahr]

If you do want to share information across multiple instances of a class, a static member is the way to go. For sure, you then need to handle synchronization issues in multi-threaded environments.

You're right, we should probably exclude these exceptions from the clang-tidy check 👍 . What do you think about adding a dedicated naming convention for static variables like s_?

Why, in first place, you want to change free functions to lower_case style? This will invalidate all existing free functions, thus also breaking API.

It is not about changing but unifying sometimes we use lower_case (here or here) sometimes upperCase (here or here). I don't have a strong opinion about which rule to enforce (maybe camelCased to follow the ROS style guide), but we should be consistent with it.

Note, that clang-tidy runs on code changes only by default.

I am aware of that but thanks for the hint! What is the best way to run the checks globally once to enforce a new (and probably existing ones too 😅 ) rule everywhere?

[henningkayser]

Just a note on using snake case vs camel case. The ROS wiki does not apply to ROS 2 anymore and pretty much all of ROS core is using snake case to basically follow what the C++ std library is doing. There is a note on that in the ROS 2 docs. I don't really have a strong opinion on which version is better, but we should certainly be consistent about it, and it would probably not be wrong to be consistent with the ROS 2 ecosystem and the std libs that we use.

Also, this is pretty funny (from ROS 2 docs):

reason: either an historical oversight or a personal preference that didn’t get checked by the linter

... on static members:
We are not really debating removing them, are we? This PR is really only about naming conventions. I don't really agree that we should not use static (I personally use it pretty frequently), even though there are often better alternatives for static class members that synchronize the information in a more verbose or thread-safe way. We should use those alternatives whenever possible.

[rhaschke]

It is not about changing but unifying sometimes we use lower_case (here or here) sometimes upperCase (here or here).

Our existing style rules (at least for MoveIt 1) enforced camelCase function names. The source locations you pointed out using snake_case, @sjahr, where only recently added to MoveIt (one of them by you)! I fully agree of being consistent. But just change all wrong spelling to camelCase 😉

What is the best way to run the checks globally once to enforce a new rule everywhere?

Manually trigger the job on github. That's at least what I configured for MoveIt1 several years ago.

... on static members: We are not really debating removing them, are we?

@sjahr raised exactly that question in his original post. And I argued strongly against it. Meanwhile, the wording was changed from "remove" to "avoid" static members.

[ChrisThrasher]

I'm not aware of the technical details, but somehow the CI pipeline is telling clang-tidy to only run on whatever files changed in a given PR which means that changes to only .clang-tidy result in clang-tidy simply not being re-ran. Is there any way to make a special rule that any changes to .clang-tidy results in clang-tidy being ran across the entire codebase? It's simply not viable to change the .clang-tidy file if you don't re-run clang-tidy across the entire codebase to see the impact of the new rules.

[rhaschke]

Please don't add prefixes or suffixes to variable names. Marking static variables via UPPER_CASE should be sufficient.

[tylerjw]

What about the case where the static variable is mutable and not a constant?

[rhaschke]

So far, we just had lower_case names for them.

[sjahr]

Our conclusion in the maintainer meeting was that we'd like to mark mutable static variables due to their impact on thread safety. Having a unique naming rule will give them more visibility for reviewers and developers. However, there was no strong preference for a specific naming rule. What would be your reasoning against a unique naming rule and if you don't have any objections, which naming convention would you prefere?

[rhaschke]

If you want to mark mutable static variables, using a prefix s_ is probably the way to go. Note that this will require many code changes, which all should be part of this PR.

[tylerjw]

I hope that, generally, we don't use mutable static variables except in some generic code for edge cases. I don't know if that is true though :/

[github-actions]

This PR is stale because it has been open for 45 days with no activity. Please tag a maintainer for help on completing this PR, or close it if you think it has become obsolete.

[mergify]

This pull request is in conflict. Could you fix it @sjahr?

[tylerjw]

@sjahr do you plan on picking back up this work? I'm sorry this got stale.

[tylerjw]

yay snakeCase, maybe over time I'll learn to like it