Format includes with clang-format

[tylerjw]

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: https://github.com/ros-planning/moveit2/pull/583

[codecov[bot]]

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]

We really need to get https://github.com/ros-planning/moveit/pull/2792 fixed so we can proceed with many other PRs....

[tylerjw]

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

[tylerjw]

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

I learned this by poking around in some google code.

[tylerjw]

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]

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]

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]

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...