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

Melodic clang naming #1185

Merged

Conversation

BryceStevenWilley
Copy link
Contributor

Description

Replaces #917 by targeting melodic.

Gave clang-tidy some teeth when it comes to identifier naming. Conventions are based on what was proposed in #814 with a few additions and exceptions:

  • Lots of specific rules for constants (local and parameter constants are treated like normal variables, but static constants are UPPER_CASE)
  • Manually had to fix some variable collisions that would have happened (local variables R and r, both become r). Gave better names as best as I could.

Something to consider is that changing the class methods possibly breaks API, even though they're being changed to conform with ROS standards. All of those changes are contained in the 2nd-to-last commit and can be easily reverted though.

The code is the only good documentation for this clang-tidy feature right now, so see here for the list of supported identifier types and here for the case types. We can also add arbitrary prefixes and suffixes to any identifier.

Checklist

  • Required by CI: Code is auto formatted using clang-format
  • Document API changes relevant to the user in the moveit/MIGRATION.md notes

@BryceStevenWilley BryceStevenWilley mentioned this pull request Nov 3, 2018
5 tasks
@davetcoleman
Copy link
Member

This is great!

@davetcoleman
Copy link
Member

Two questions:

  • This is only applied to moveit_core, correct? You think it should be applied everywhere soon?
  • Can you make a continuous integration auto-enforcement rule for this? (future PR is fine)

@BryceStevenWilley
Copy link
Contributor Author

  • This is only applied to moveit_core, yes. The next PR will be clang tidy applied to all of moveit, and then I'll make one more PR that has a bunch of manual changes like these, to handle loops and inconsistent parameter names as per this conversation.
  • Once clang-tidy is applied to all of moveit, the plan is to make a CI tool for this, yes. I'm not quite sure how to do that yet, but it will be the last PR to cap these changes off.

@davetcoleman davetcoleman merged commit 1e7023b into moveit:melodic-devel Nov 9, 2018
@davetcoleman davetcoleman mentioned this pull request Feb 21, 2019
1 task
@BryceStevenWilley BryceStevenWilley deleted the melodic-clang-naming branch March 11, 2019 23:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants