Merged
Conversation
This was referenced Nov 18, 2016
Member
Author
|
Note: Travis will fail for clang-format test until I upgrade
|
Member
Author
|
ping - any thoughts on this? |
v4hn
approved these changes
Nov 24, 2016
Contributor
v4hn
left a comment
There was a problem hiding this comment.
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!
This was referenced Nov 24, 2016
Member
Author
|
I will merge all three PRs once I verify the moveit_ci one is working |
d6e7618 to
9cadf5a
Compare
Member
Author
|
I have just re-run clang-format 3.8 on the latest repo and am ready to merge this when CI passes |
9cadf5a to
d44ff4f
Compare
Contributor
|
@davetcoleman I assume we should also upgrade the indigo and jade branches? |
This was referenced Jan 6, 2017
Member
Author
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
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:
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: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