Format includes with clang-format #2795

tylerjw · 2021-07-31T06:14:05Z

Description

@v4hn this was much easier in moveit than in moveit2... there were only 2 files I found with missing includes.

The gist of this change is that it formats the include lines so we can stop doing it manually. I don't like manually re-organizing include lines and think clang-format should do this for me. I hope you like this change as much as I do.

Here is the moveit2 PR this is a port of: moveit/moveit2#583

codecov · 2021-07-31T08:19:24Z

Codecov Report

Merging #2795 (f610415) into master (86174f3) will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2795      +/-   ##
==========================================
- Coverage   60.44%   60.43%   -0.00%     
==========================================
  Files         366      366              
  Lines       31673    31675       +2     
==========================================
+ Hits        19140    19141       +1     
- Misses      12533    12534       +1

Impacted Files	Coverage Δ
...n/allvalid/collision_detector_allocator_allvalid.h	`100.00% <ø> (ø)`
...lude/moveit/collision_detection/collision_common.h	`93.03% <ø> (ø)`
...include/moveit/collision_detection/collision_env.h	`100.00% <ø> (ø)`
...lude/moveit/collision_detection/collision_matrix.h	`100.00% <ø> (ø)`
...include/moveit/collision_detection/occupancy_map.h	`81.25% <ø> (ø)`
.../collision_detection/test_collision_common_panda.h	`94.08% <ø> (ø)`
...it/collision_detection/test_collision_common_pr2.h	`98.12% <ø> (ø)`
...tection/include/moveit/collision_detection/world.h	`100.00% <ø> (ø)`
..._detection/src/allvalid/collision_env_allvalid.cpp	`7.15% <ø> (ø)`
...eit_core/collision_detection/src/collision_env.cpp	`72.08% <ø> (ø)`
... and 268 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 86174f3...f610415. Read the comment docs.

v4hn · 2021-08-06T11:37:13Z

We really need to get #2792 fixed so we can proceed with many other PRs....

tylerjw · 2021-08-10T18:40:50Z

@v4hn I updated this with your suggested change in the PR to moveit2.

rhaschke

I like this very much! Didn't know you can define include groups.

tylerjw · 2021-08-10T19:36:52Z

I like this very much! Didn't know you can define include groups.

I learned this by poking around in some google code.

v4hn

This is a pain to review (I managed a quarter before I gave up) and I'm sure we will notice unintuitive includes because of this patch in the future...

v4hn · 2021-08-13T11:17:44Z

...detection_bullet/include/moveit/collision_detection_bullet/bullet_integration/bullet_utils.h


 #pragma once

 #include <btBulletCollisionCommon.h>


This registers as a false-positive because bullet seems to use includes without prefix. Not much we can do about it I think, or do we want to add exceptions for specific header names?

What do you mean by false-positive here?

this includes bullet headers, but it's grouped with standard headers because there is no prefix used in the include path

Got it. However, before the bullet header was not grouped with anything else as well...

v4hn · 2021-08-13T11:20:13Z

moveit_core/collision_detection_bullet/src/collision_env_bullet.cpp

-#include <moveit/collision_detection_bullet/bullet_integration/contact_checker_common.h>
 #include <boost/bind.hpp>
+
 #include <bullet/btBulletCollisionCommon.h>


or, we just update all bullet includes to use the bullet/ prefix. Afaik the bullet tutorials use includes without prefix though.

v4hn · 2021-08-13T11:22:19Z

moveit_core/collision_distance_field/src/collision_env_distance_field.cpp

+#include <boost/bind.hpp>
+
 #include <ros/assert.h>
+#include <ros/console.h>


I would expect to see all ros/ includes before any other ros package.

tylerjw · 2021-08-13T14:30:51Z

This is a pain to review (I managed a quarter before I gave up) and I'm sure we will notice unintuitive includes because of this patch in the future...

More generally what I wanted was some sort of automated sorting of these. I might be weird in that I don't really care if they create some odd orders, just that there is some order. I want consistency in this and to stop getting review comments asking me to manually sort these.

If anything I think we should adopt a simpler set of rules than these that are essentially:

includes without / (mostly system includes)
includes with / in them
includes that start with the word moveit
includes that use local search "

v4hn · 2021-08-13T18:05:31Z

If anything I think we should adopt a simpler set of rules than these that are essentially:

then we might as well go for simple alphabetical order altogether. include order should not matter aside from aesthetics. all the things I've pointed out so far are issues I would be inclined to change if I'd encounter them in source code.

to stop getting review comments asking me to manually sort these.

If that's your only reason for trying to enforce a syntactic order on (semantic) includes I would say people should stop criticizing it unless there is no obvious order in the patch whatsoever.

If people look at the autosorted includes later and have to adjust the clang-format file to get them in a meaningful order that's just what I would call "verschlimmbessern" in German...

rhaschke · 2021-08-15T15:19:26Z

then we might as well go for simple alphabetical order altogether. include order should not matter aside from aesthetics. all the things I've pointed out so far are issues I would be inclined to change if I'd encounter them in source code.

I don't get your criticism yet. Aren't you one of the guys who prefer sorting e.g. for package dependencies as well?
This PR just helps to avoid redundant includes and provides a nice grouping of similar includes.
I hate simple alphabetical sorting because it destroys any semantic grouping which was introduced manually. I think this PR is a very nice compromise.

If people look at the autosorted includes later and have to adjust the clang-format file to get them in a meaningful order that's just what I would call "verschlimmbessern" in German...

I agree that we should come up with a rule set to push the includes into a meaningful order before merging this PR.
If you don't like the current rules, please list what you want to be changed. From your specific comments, I cannot yet infer any desired changes.

v4hn · 2021-08-15T16:02:08Z

that statement was mainly a reply to

I don't really care if they create some odd orders, just that there is some order. I want consistency in this and to stop getting review comments asking me to manually sort these.

generally I think the approach is worthwhile, but it's a hassle to look through all the changes and judge them.
I gave multiple rounds of feedback already.
in the last review I mainly noticed the bullet includes we either want to allow exceptions for or modify to use the bullet/ prefix.

Also I would expect another group for ros/ includes.

there is probably more to discuss in the rest of the patch...

tylerjw · 2021-08-17T15:18:57Z

moveit_core/robot_model/include/moveit/robot_model/robot_model.h

 #pragma once

-#include <moveit/macros/class_forward.h>
-#include <moveit/exceptions/exceptions.h>
 #include <urdf/model.h>
+
 #include <srdfdom/model.h>

+#include <moveit/exceptions/exceptions.h>
+#include <moveit/macros/class_forward.h>
+
 // joint types
-#include <moveit/robot_model/joint_model_group.h>
+#include <iostream>
+
+#include <Eigen/Geometry>
+
 #include <moveit/robot_model/fixed_joint_model.h>
 #include <moveit/robot_model/floating_joint_model.h>
+#include <moveit/robot_model/joint_model_group.h>
 #include <moveit/robot_model/planar_joint_model.h>
-#include <moveit/robot_model/revolute_joint_model.h>
 #include <moveit/robot_model/prismatic_joint_model.h>
-
-#include <Eigen/Geometry>
-#include <iostream>
+#include <moveit/robot_model/revolute_joint_model.h>


One of the challenging things about this PR is that comments above blocks of includes essentially breaks the re-rodering. Here it re-ordered above and below the // joint types line as separate blocks. I've gone through and removed a bunch of these comments but here is one I forgot.

tylerjw requested review from henningkayser, jspricke, rhaschke and v4hn July 31, 2021 06:14

tylerjw requested review from AndyZe, felixvd, gavanderhoorn, jrgnicho, jschleicher, mamoll, mikeferguson, mlautman and nbbrooks as code owners July 31, 2021 06:14

tylerjw force-pushed the format-includes branch from bd00827 to f73ec37 Compare August 10, 2021 18:40

tylerjw force-pushed the format-includes branch from f73ec37 to 741137e Compare August 10, 2021 18:43

rhaschke approved these changes Aug 10, 2021

View reviewed changes

tylerjw force-pushed the format-includes branch from 741137e to 1335020 Compare August 10, 2021 20:53

Format includes with clang-format

f610415

v4hn force-pushed the format-includes branch from 1335020 to f610415 Compare August 13, 2021 11:11

v4hn reviewed Aug 13, 2021

View reviewed changes

v4hn mentioned this pull request Aug 13, 2021

more fixes for the clang-tidy job #2813

Merged

tylerjw commented Aug 17, 2021

View reviewed changes

rhaschke force-pushed the master branch 5 times, most recently from e1b0ec1 to fdc3b1c Compare July 28, 2023 10:37

Format includes with clang-format #2795

Are you sure you want to change the base?

Format includes with clang-format #2795

Uh oh!

Conversation

tylerjw commented Jul 31, 2021

Description

Uh oh!

codecov bot commented Jul 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

v4hn commented Aug 6, 2021

Uh oh!

tylerjw commented Aug 10, 2021

Uh oh!

rhaschke left a comment

Choose a reason for hiding this comment

Uh oh!

tylerjw commented Aug 10, 2021

Uh oh!

v4hn left a comment

Choose a reason for hiding this comment

Uh oh!

v4hn Aug 13, 2021

Choose a reason for hiding this comment

Uh oh!

rhaschke Aug 15, 2021

Choose a reason for hiding this comment

Uh oh!

v4hn Aug 15, 2021

Choose a reason for hiding this comment

Uh oh!

rhaschke Aug 15, 2021

Choose a reason for hiding this comment

Uh oh!

v4hn Aug 13, 2021

Choose a reason for hiding this comment

Uh oh!

v4hn Aug 13, 2021

Choose a reason for hiding this comment

Uh oh!

tylerjw commented Aug 13, 2021

Uh oh!

v4hn commented Aug 13, 2021

Uh oh!

rhaschke commented Aug 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

v4hn commented Aug 15, 2021

Uh oh!

tylerjw Aug 17, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Jul 31, 2021 •

edited

Loading

rhaschke commented Aug 15, 2021 •

edited

Loading