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

Fix Kinetic clang format #367

Merged
merged 1 commit into from Jan 5, 2017

Conversation

davetcoleman
Copy link
Member

This is similar to @v4hn's #344 except it only upgrades us to 3.8 since that is still included in Ubuntu Trusty (ROS Indigo/Jade) but 3.9 is not

I had to apply some new settings to keep the formatting almost the same, still there are some changes that despite an hour of searching I could not figure out how to disable. Also included are some formatting fixes that were evidently a bug in 3.6:

Added:

SortIncludes: false
SpaceAfterCStyleCast: false
BreakBeforeBraces: Custom
BraceWrapping: {
    AfterClass: 'true'
    AfterControlStatement: 'true'
    AfterEnum : 'true'
    AfterFunction : 'true'
    AfterNamespace : 'true'
    AfterStruct : 'true'
    AfterUnion : 'true'
    BeforeCatch : 'true'
    BeforeElse : 'true'
    IndentBraces : 'false'
}

In addition, this PR fixes a bug in our current setup as pointed out by @bmagyar here that pointer/reference symbol alignment to the variable type is inconsistent - sometimes it is aligned to the variable name instead of type. Upon investigation I discovered it is because we had the setting DerivePointerBinding: true - meaning:

If true, analyze the formatted file for the most common binding and use PointerBindsToType only as fallback.

I've disabled this so that all pointers are aligned to type. This is not explicitly specified in the ROSCpp style guidelines and according to the Google style guidelines it does not matter which you use, but looking at ros_comm they use the standard of type aligned which I prefer. I think we should be consistent throughout MoveIt!.

@rbbg @v4hn

@davetcoleman
Copy link
Member Author

Note: Travis will fail for clang-format test until I upgrade moveit_ci, which I'll do after we discuss/confirm this PR

  • Update version of clang-format for moveit_ci
  • Update version of clang-format on moveit.ros.org

@davetcoleman
Copy link
Member Author

ping - any thoughts on this?

Copy link
Contributor

@v4hn v4hn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delays...

I like it! There is no reason to hang on to 3.6 if 3.8 is already available in trusty.
Please go ahead!

@davetcoleman
Copy link
Member Author

I will merge all three PRs once I verify the moveit_ci one is working

@davetcoleman
Copy link
Member Author

I have just re-run clang-format 3.8 on the latest repo and am ready to merge this when CI passes

@v4hn v4hn merged commit 4bc43c5 into moveit:kinetic-devel Jan 5, 2017
@v4hn
Copy link
Contributor

v4hn commented Jan 5, 2017

@davetcoleman I assume we should also upgrade the indigo and jade branches?
Are you going to do that?

@davetcoleman davetcoleman deleted the kinetic-clang-format branch January 6, 2017 06:29
@davetcoleman
Copy link
Member Author

Yes
#403
#404

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

2 participants