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

Improve clang format #1193

Merged
merged 5 commits into from
Nov 21, 2018
Merged

Conversation

rhaschke
Copy link
Contributor

@rhaschke rhaschke commented Nov 12, 2018

This is an attempt to improve the results of clang-format. Particularly:

  • increase penalty to break literal strings
  • increase penalty to break before first argument in function calls
  • reduce penalty to put return type on its own line

I think the resulting code is much better readable.

addBackgroundJob(boost::bind(&MotionPlanningDisplay::publishInteractiveMarkers, this, false), "publishInteractiveMark"
"ers");
addBackgroundJob(boost::bind(&MotionPlanningDisplay::publishInteractiveMarkers, this, false),
"publishInteractiveMarkers");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Splitting literal strings like this was awful.

@jonbinney
Copy link
Contributor

+1 for this change.

@rhaschke
Copy link
Contributor Author

Rebased / reapplied settings to resolve conflicts.

Copy link
Member

@davetcoleman davetcoleman left a comment

Choose a reason for hiding this comment

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

I'm not convinced this minor tweak is worth the code disruption of other PRs... some of the changes I like, but most are not any better and some are worse. I'm fine with being outvoted on this, however.

active_joint_model_vector_[i]->getVariableRandomPositionsNearBy(rng, values + active_joint_model_start_index_[i],
*active_joint_bounds[i],
near + active_joint_model_start_index_[i],
distance);
Copy link
Member

Choose a reason for hiding this comment

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

i prefer the old version - this change is less readable IMO and adds another line.

active_joint_model_vector_[i]->getVariableRandomPositionsNearBy(rng, values + active_joint_model_start_index_[i],
*active_joint_bounds[i],
near + active_joint_model_start_index_[i],
distances[i]);
Copy link
Member

Choose a reason for hiding this comment

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

same

@rhaschke
Copy link
Contributor Author

I agree that this PR should only be merged after other large pending PRs.
@davetcoleman, Obviously you don't mind to have a newline directly after the opening parenthesis of a function call. By reducing the penalty for this, we can easily get this behaviour back.
I think, the key contribution of this PR is to avoid breaking literal strings.

@rhaschke
Copy link
Contributor Author

@davetcoleman Could you please have another look. I further tweaked the clang-format settings, such that the source lines you complained about are not touched anymore.
The reformatting only touches a few lines now. So there is no reason to hold this back anymore.

Copy link
Member

@davetcoleman davetcoleman left a comment

Choose a reason for hiding this comment

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

It does look better now!

@rhaschke rhaschke merged commit 4c2b4ba into moveit:melodic-devel Nov 21, 2018
@rhaschke rhaschke deleted the improve-clang-format branch November 21, 2018 07:17
@rhaschke rhaschke mentioned this pull request Nov 21, 2018
rhaschke added a commit that referenced this pull request Nov 21, 2018
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