Force include order via clang-format

[SunBlack]

Open for discussion for the include order.

My idea, as somewhere already was mentioned that pcl => 3rd-Party = std was wished:

1. PCL
2. GoogleTest, as it will be used only in tests
3. Main 3rd-Party components:
   1. boost
   2. eigen
   3. flann
4. Main 3rd-Party components of apps
   1. qt
   2. vtk
5. Minor 3rd-Party components
   1. librealsense2
   2. ros (message_filters is part of it)
   3. opencv
   4. thrust
   5. OpenGL/GLUT
6. stdlib

Was first thinking about general alphabetical order of 3rd-party components, but I believe e.g. boost should stay more near pcl than other.

Furthermore I use (each 3rd-Party component separated):

#include <pcl/..
#include <pcl/..

#include <boost/..
#include <boost/..

#include <flann/..

#include <thrust/...

#include <map>

and not (since clang-format 10 it is even possible via SortPriority to change order within a group)

#include <pcl/..
#include <pcl/..

#include <boost/..
#include <boost/..
#include <flann/..
#include <thrust/...

#include <map>

Further hint: If there is a macro between includes (e.g. a #if) clang-format will not join the include lists before and after it.

[kunaltyagi]

I like the separated version

[SunBlack]

Oh I forgot the ui_* files of Qt. Should they stay at beginning or end of the list?

[kunaltyagi]

Oh I forgot the ui_* files of Qt. Should they stay at beginning or end of the list?

Why not organize them with Qt using SortPriority? Let them be alphabetically in the list and if it doesn't make sense, then we shall come back to modify the list

[SunBlack]

Why not organize them with Qt using SortPriority? Let them be alphabetically in the list and if it doesn't make sense, then we shall come back to modify the list

On this way the .clang-format file cannot be used anymore by clang-format 7-9. Just to mention ;-).

[kunaltyagi]

In that case, let's modify the documentation to refer to clang-format-10 or higher exclusively, just in case this merges.

[SunBlack]

From my point the order is finished. Should I start to fix the include order, where a macro is inside? Or wait for response of @taketwo and @SergioRAgostinho?

[SergioRAgostinho]

Feel free to go ahead.

[kunaltyagi]

Shall we clean up the headers (split due to comments, macros, etc.) and the "" -> <> conversion here?

[SergioRAgostinho]

Shall we clean up the headers (split due to comments, macros, etc.) and the "" -> <> conversion here?

In this PR I have no strong preference. Although personally I would probably just merge this one and open a new one.

[SunBlack]

Just to mention: I remove the definition of VTK_EXCLUDE_STRSTREAM_HEADERS, as this is not anymore used since VTK 6.0 (see here).

[SunBlack]

So I hope this is now okay.

Just a hint: in <pcl/simulation/tool/...> there may be includes with #include "". This is not a fix we should do in this MR, as there is no correct include directory set, so we cannot use #include <pcl/simulation/tools/...> instead (and as mentioned before in another comment: there is a difference between #include <> and #include "")

[SunBlack]

Just mention: Ordering of Qt includes seems to have a bit trouble. I'm checking it, so please wait with merging (ui-files are before Qt files instead behind).

The issue:

IncludeCategories:
  - Regex:           '^<pcl/'
    Priority:        1
...
  - Regex:           '^<(Q|q)[^/]+>'
    Priority:        6
    SortPriority:    0
  - Regex:           '^<ui_[^/]+\.h>$'
    Priority:        6
    SortPriority:    1
...

leads to:

#include <pcl/common/angles.h>

#include <ui_manual_registration.h>
#include <QMainWindow>
#include <QMutex>
#include <QTimer>

Expected was that the UI-files are before the Qt files

So I adjusted the SortPriority:

IncludeCategories:
  - Regex:           '^<pcl/'
    Priority:        1
...
  - Regex:           '^<(Q|q)[^/]+>'
    Priority:        6
    SortPriority:    1
  - Regex:           '^<ui_[^/]+\.h>$'
    Priority:        6
    SortPriority:    0
...

leads to:

#include <QMainWindow>
#include <QMutex>
#include <QTimer>

#include <pcl/common/angles.h>

#include <ui_manual_registration.h>

This is broken.

A working example is (but then it seems SortPriority depends on Priority):

IncludeCategories:
  - Regex:           '^<pcl/'
    Priority:        1
...
  - Regex:           '^<(Q|q)[^/]+>'
    Priority:        6
    SortPriority:    6
  - Regex:           '^<ui_[^/]+\.h>$'
    Priority:        6
    SortPriority:    7
...

leads to:

#include <pcl/common/angles.h>

#include <QMainWindow>
#include <QMutex>
#include <QTimer>
#include <ui_manual_registration.h>

//Edit: Fixed

[kunaltyagi]

SortPriority is related to priority and is not independent of it.

Since upper case letters are "less" than lower case letters, just priority 6 for both groups should have resulted in the same output as with sort-priorities set.

[SunBlack]

So don't use SortPriority, when usual ordering is already enough? This would allow clang-format 7-9 again, if I prefer if we make old version unusable, as the give another result.

[kunaltyagi]

I prefer if we make old version unusable

That is already the case with the cmake script modification, right? If less configuration works, then we should not add more. (And our CI will catch those trying to be sneaky)

[aPonza]

FYI we're partially overlapping (yours is more complete, I was touching less files for other reasons) on the VTK define removal in #3893. Maybe I shall revert my commit entirely?

[SunBlack]

@aPonza: This PR should be ready to merge, so yeah, no need to have it in your PR too.

@kunaltyagi I removed SortPriority. Neverthless I didn't combined them in one regex as <Q[^/]+|ui_[^/]+\.h>$ is not better readable.

[SunBlack]

Seems I have to redo the whole PR again, as #3893 reordered includes, so I will have Merge conflicts in each step of this PR again -.-

[aPonza]

Oops, I can make a patch for you.

[SunBlack]

I will split some parts of it now in separate PRs, as I belueve it is easier to run clang-format again on the new base.

[SunBlack]

As I see this:

pcl/simulation/include/pcl/simulation/range_likelihood.h

Lines 3 to 8 in 98e3eed

	#include <pcl/common/transforms.h>
	#include <pcl/memory.h>
	#include <pcl/pcl_config.h>
	#include <pcl/pcl_macros.h>
	#include <pcl/range_image/range_image_planar.h>
	#include <pcl/simulation/camera.h>

May it be useful to change the pcl ordering from just

  - Regex:           '^<pcl/'
    Priority:        1

to

  - Regex:           '^<pcl/[^/]+>$'
    Priority:        1
    SortPriority:    1
  - Regex:           '^<pcl/[^/]+/[^/]+>$'
    Priority:        1
    SortPriority:    2
  - Regex:           '^<pcl/([^/]+/){2}[^/]+>$'
    Priority:        1
    SortPriority:    3
  - Regex:           '^<pcl/([^/]+/){3,}[^/]+>$'
    Priority:        1
    SortPriority:    4

?

I think this should change this to:

 #include <pcl/memory.h> 
 #include <pcl/pcl_config.h> 
 #include <pcl/pcl_macros.h> 
 #include <pcl/common/transforms.h> 
 #include <pcl/range_image/range_image_planar.h> 
 #include <pcl/simulation/camera.h> 

[SergioRAgostinho]

I think this should change this to:

 #include <pcl/memory.h> 
 #include <pcl/pcl_config.h> 
 #include <pcl/pcl_macros.h> 
 #include <pcl/common/transforms.h> 
 #include <pcl/range_image/range_image_planar.h> 
 #include <pcl/simulation/camera.h> 

The other way around. The more fundamental headers come last.

new work added

[SunBlack]

Don't thing this is a good idea, as you would now expect, as you have then:

1. More nested directories first

#include <pcl/ml/dt/decision_forest.h>
#include <pcl/ml/dt/decision_tree_evaluator.h>
#include <pcl/common/common.h>
#include <pcl/console/parse.h>
#include <pcl/ml/branch_estimator.h>
#include <pcl/ml/feature_handler.h>
#include <pcl/ml/stats_estimator.h>
#include <pcl/memory.h> 
#include <pcl/pcl_config.h> 
#include <pcl/pcl_macros.h>

2. Sort just be modules and without module name in path as last

#include <pcl/common/common.h>
#include <pcl/console/parse.h>
#include <pcl/ml/branch_estimator.h>
#include <pcl/ml/dt/decision_forest.h>
#include <pcl/ml/dt/decision_tree_evaluator.h>
#include <pcl/ml/feature_handler.h>
#include <pcl/ml/stats_estimator.h>
#include <pcl/memory.h> 
#include <pcl/pcl_config.h> 
#include <pcl/pcl_macros.h>

As you can see both variants are ugly:

1. submodules are getting mixed
2. subdirectories are between files listening (modules are even splitted, as pcl/console/parse.h is within the common module)

Without prefering to have the main include below we could have (EDIT: Got trapped by myself - this has the same issue as 1) - only not visible as I have subdirectories only within one module in this example )

#include <pcl/memory.h> 
#include <pcl/pcl_config.h> 
#include <pcl/pcl_macros.h>
#include <pcl/common/common.h>
#include <pcl/console/parse.h>
#include <pcl/ml/branch_estimator.h>
#include <pcl/ml/feature_handler.h>
#include <pcl/ml/stats_estimator.h>
#include <pcl/ml/dt/decision_forest.h>
#include <pcl/ml/dt/decision_tree_evaluator.h>

In general I prefer last one, since the sorting of the other two variants is not comprehensible at first glance. Or is there any real reason, why the major header should be at end?

//Edit: Just why I prefer files before files in subdirectory:

If I'm looking into a directory I first watch all files in this directory before I going into subdirectory. This is like

nano branch_estimator.h
nano denserf.h
nano feature_handler.h
nano kmeans.h
...
ls dt
nano dt/..
ls fern
nano fern/...

vs

nano branch_estimator.h
nano denserf.h
ls dt
nano dt/..
nano feature_handler.h
ls fern
nano fern/...
nano kmeans.h
...

[SergioRAgostinho]

Or is there any real reason, why the major header should be at end?

It helps find issues with transitive includes early. Imagine pcl/common/common.h relies on pcl/memory.h and for some reason does not include it. In another source file you included them in the order you suggested, the more fundamental/major gets included first.

pcl/memory.h
pcl/common/common.h

common.h has a problem but you still did not pick up on it, because your are consistently including pcl/memory.h before pcl/common/common.h. The other way around it is impossible for that to happen.

[SunBlack]

In this case: Wouldn't it be better to sort even modules by dependencies? E.g.

#include <pcl/io/...>
#include <pcl/octree/...>
#include <pcl/common/...>
#include <pcl/...>

[SergioRAgostinho]

Ideally yes, but I am already happy with the current alphabetical + folder depth approach.

[SunBlack]

Is there any overall doxygen graph with the dependencies between the modules?

@folder depth: This would increase size of .clang-format a lot:

  - Regex:           '^<pcl/io/[^/]+>$'
    Priority:        100
    SortPriority:    113
  - Regex:           '^<pcl/io(/[^/]+){1}>$'
    Priority:        100
    SortPriority:    112
  - Regex:           '^<pcl/io(/[^/]+){2}>$'
    Priority:        100
    SortPriority:    111
  - Regex:           '^<pcl/io(/[^/]+){3}>$'
    Priority:        100
    SortPriority:    110

  - Regex:           '^<pcl/octree/[^/]+>$'
    Priority:        100
    SortPriority:    123
  - Regex:           '^<pcl/octree(/[^/]+){1}>$'
    Priority:        100
    SortPriority:    122
  - Regex:           '^<pcl/octree(/[^/]+){2}>$'
    Priority:        100
    SortPriority:    121
  - Regex:           '^<pcl/octree(/[^/]+){3}>$'
    Priority:        100
    SortPriority:    120

  - Regex:           '^<pcl/common/[^/]+>$'
    Priority:        100
    SortPriority:    133
  - Regex:           '^<pcl/common(/[^/]+){1}>$'
    Priority:        100
    SortPriority:    132
  - Regex:           '^<pcl/common(/[^/]+){2}>$'
    Priority:        100
    SortPriority:    131
  - Regex:           '^<pcl/common(/[^/]+){3}>$'
    Priority:        100
    SortPriority:    130

  - Regex:           '^<pcl(/[^/]+){3}>$'
    Priority:        100
    SortPriority:    199

And this is just for 3 modules

[SergioRAgostinho]

Is there any overall doxygen graph with the dependencies between the modules?

I don't know the answer to this.

folder depth: This would increase size of .clang-format a lot:

Let me clarify: I'm already very happy with the variant 2 you presented in your comment. I don't think we need more than that.

[SunBlack]

Let me clarify: I'm already very happy with the variant 2 you presented in your comment. I don't think we need more than that.

😆

I am already happy with the current alphabetical + folder depth approach.

This was variant 1 ;-)

But yeah, maybe start with variant 2. All other is getting to complex, as long as clang-format doesn't have more options.

[kunaltyagi]

Simpler and easily understandable by human (eg: put in alphabetical order, or sort by module, etc.) is better.

[SunBlack]

Adjusted:

• More fundamental headers come last
• Increase value of Priority in .clang-format so in case a new dependency will be added not all values have to be adjusted

I didn't added grouped ordering by submodule dependencies, as this list may be incomplete (e.g. easy to forget pcl/compression is part of module io)