Force include order via clang-format #3909

SunBlack · 2020-04-11T23:44:48Z

Open for discussion for the include order.

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

PCL
GoogleTest, as it will be used only in tests
Main 3rd-Party components:
1. boost
2. eigen
3. flann
Main 3rd-Party components of apps
1. qt
2. vtk
Minor 3rd-Party components
1. librealsense2
2. ros (message_filters is part of it)
3. opencv
4. thrust
5. OpenGL/GLUT
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 · 2020-04-11T23:53:01Z

I like the separated version

.clang-format

SunBlack · 2020-04-12T10:53:01Z

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

.clang-format

simulation/tools/sim_viewer.cpp

kunaltyagi · 2020-04-13T06:45:09Z

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 · 2020-04-13T11:53:12Z

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 · 2020-04-13T13:17:33Z

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

SunBlack · 2020-04-13T13:39:10Z

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.

ml/include/pcl/ml/permutohedral.h

octree/include/pcl/octree/octree_nodes.h

kunaltyagi

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

simulation/include/pcl/simulation/model.h

simulation/include/pcl/simulation/range_likelihood.h

simulation/include/pcl/simulation/sum_reduce.h

simulation/src/range_likelihood.cpp

simulation/tools/sim_terminal_demo.cpp

simulation/tools/sim_test_performance.cpp

simulation/tools/sim_test_simple.cpp

SergioRAgostinho · 2020-04-14T12:21:13Z

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 · 2020-04-14T15:10:46Z

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 · 2020-04-14T20:50:15Z

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 "")

simulation/tools/sim_viewer.cpp

SunBlack · 2020-04-15T14:08:26Z

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 · 2020-04-15T15:31:43Z

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 · 2020-04-15T17:24:47Z

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 · 2020-04-16T00:22:32Z

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)

SunBlack · 2020-04-17T11:59:30Z

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 · 2020-04-17T12:15:18Z

Oops, I can make a patch for you.

SunBlack · 2020-04-17T12:19:05Z

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 · 2020-04-20T17:25:22Z

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 · 2020-04-20T22:08:46Z

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 · 2020-04-21T10:01:02Z

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

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>

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:

submodules are getting mixed
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 · 2020-04-21T11:08:02Z

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 · 2020-04-21T11:56:38Z

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 · 2020-04-21T12:10:02Z

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

SunBlack · 2020-04-21T12:18:00Z

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 · 2020-04-21T13:02:35Z

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 · 2020-04-21T13:40:42Z

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 · 2020-04-21T14:21:34Z

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

* Increase value of Priority in .clang-format so in case a new dependency will be added not all values have to adjusted

SunBlack · 2020-04-21T14:40:34Z

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)

.clang-format

Use PointCloudLibrary#3909 .clang-format

SunBlack mentioned this pull request Apr 11, 2020

[docs] Ordering of header includes #3904

Closed

SunBlack force-pushed the clang-format_include_order branch from 941abdc to 6c2c3ed Compare April 11, 2020 23:48

kunaltyagi reviewed Apr 12, 2020

View reviewed changes

.clang-format Outdated Show resolved Hide resolved

diivm reviewed Apr 12, 2020

View reviewed changes

.clang-format Outdated Show resolved Hide resolved

diivm reviewed Apr 12, 2020

View reviewed changes

simulation/tools/sim_viewer.cpp Outdated Show resolved Hide resolved

simulation/tools/sim_viewer.cpp Outdated Show resolved Hide resolved

simulation/tools/sim_viewer.cpp Outdated Show resolved Hide resolved

kunaltyagi added changelog: new feature Meta-information for changelog generation needs: feedback Specify why not closed/merged yet labels Apr 13, 2020

kunaltyagi self-requested a review April 13, 2020 14:20

SergioRAgostinho approved these changes Apr 14, 2020

View reviewed changes

ml/include/pcl/ml/permutohedral.h Outdated Show resolved Hide resolved

octree/include/pcl/octree/octree_nodes.h Outdated Show resolved Hide resolved

kunaltyagi reviewed Apr 14, 2020

View reviewed changes

SunBlack force-pushed the clang-format_include_order branch from 2382a29 to 217c8e4 Compare April 14, 2020 20:56

kunaltyagi reviewed Apr 14, 2020

View reviewed changes

simulation/tools/sim_viewer.cpp Outdated Show resolved Hide resolved

kunaltyagi previously approved these changes Apr 15, 2020

View reviewed changes

kunaltyagi added needs: more work Specify why not closed/merged yet needs: code review Specify why not closed/merged yet and removed needs: feedback Specify why not closed/merged yet needs: more work Specify why not closed/merged yet labels Apr 15, 2020

SunBlack mentioned this pull request Apr 17, 2020

Remove use of VTK_EXCLUDE_STRSTREAM_HEADERS (unavailable since VTK 6.0.0) #3939

Merged

Force include order via clang-format

98e3eed

SunBlack force-pushed the clang-format_include_order branch from c9788ae to 98e3eed Compare April 20, 2020 11:06

* More fundamental headers come last

d3dedd4

* Increase value of Priority in .clang-format so in case a new dependency will be added not all values have to adjusted

kunaltyagi reviewed Apr 21, 2020

View reviewed changes

.clang-format Show resolved Hide resolved

kunaltyagi approved these changes Apr 22, 2020

View reviewed changes

SergioRAgostinho self-requested a review April 22, 2020 08:04

SergioRAgostinho approved these changes Apr 22, 2020

View reviewed changes

SergioRAgostinho merged commit 8bcde6f into PointCloudLibrary:master Apr 22, 2020

SergioRAgostinho removed the needs: code review Specify why not closed/merged yet label Apr 22, 2020

SunBlack deleted the clang-format_include_order branch April 22, 2020 09:37

diivm mentioned this pull request Apr 22, 2020

[tracking] format module by clang-format #3814

Merged

diivm added a commit to diivm/pcl that referenced this pull request Apr 22, 2020

fixup! clang-formatted tracking impl files

a1af821

Use PointCloudLibrary#3909 .clang-format

diivm added a commit to diivm/pcl that referenced this pull request Apr 22, 2020

fixup! clang formatted tracking header files

f84aab5

Use PointCloudLibrary#3909 .clang-format

SunBlack mentioned this pull request Oct 28, 2021

Fix regex detection of std includes in the clang-format file #5002

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Force include order via clang-format #3909

Force include order via clang-format #3909

SunBlack commented Apr 11, 2020 •

edited

Loading

kunaltyagi commented Apr 11, 2020

SunBlack commented Apr 12, 2020

kunaltyagi commented Apr 13, 2020 •

edited

Loading

SunBlack commented Apr 13, 2020 •

edited

Loading

kunaltyagi commented Apr 13, 2020

SunBlack commented Apr 13, 2020

SergioRAgostinho left a comment

kunaltyagi left a comment

SergioRAgostinho commented Apr 14, 2020

SunBlack commented Apr 14, 2020

SunBlack commented Apr 14, 2020

SunBlack commented Apr 15, 2020 •

edited

Loading

kunaltyagi commented Apr 15, 2020

SunBlack commented Apr 15, 2020

kunaltyagi commented Apr 16, 2020

SunBlack commented Apr 17, 2020

aPonza commented Apr 17, 2020

SunBlack commented Apr 17, 2020

SunBlack commented Apr 20, 2020

SergioRAgostinho commented Apr 20, 2020

SunBlack commented Apr 21, 2020 •

edited

Loading

SergioRAgostinho commented Apr 21, 2020

SunBlack commented Apr 21, 2020

SergioRAgostinho commented Apr 21, 2020 •

edited

Loading

SunBlack commented Apr 21, 2020 •

edited

Loading

SergioRAgostinho commented Apr 21, 2020

SunBlack commented Apr 21, 2020

kunaltyagi commented Apr 21, 2020

SunBlack commented Apr 21, 2020 •

edited

Loading

Force include order via clang-format #3909

Force include order via clang-format #3909

Conversation

SunBlack commented Apr 11, 2020 • edited Loading

kunaltyagi commented Apr 11, 2020

SunBlack commented Apr 12, 2020

kunaltyagi commented Apr 13, 2020 • edited Loading

SunBlack commented Apr 13, 2020 • edited Loading

kunaltyagi commented Apr 13, 2020

SunBlack commented Apr 13, 2020

SergioRAgostinho left a comment

Choose a reason for hiding this comment

kunaltyagi left a comment

Choose a reason for hiding this comment

SergioRAgostinho commented Apr 14, 2020

SunBlack commented Apr 14, 2020

SunBlack commented Apr 14, 2020

SunBlack commented Apr 15, 2020 • edited Loading

kunaltyagi commented Apr 15, 2020

SunBlack commented Apr 15, 2020

kunaltyagi commented Apr 16, 2020

SunBlack commented Apr 17, 2020

aPonza commented Apr 17, 2020

SunBlack commented Apr 17, 2020

SunBlack commented Apr 20, 2020

SergioRAgostinho commented Apr 20, 2020

SunBlack commented Apr 21, 2020 • edited Loading

SergioRAgostinho commented Apr 21, 2020

SunBlack commented Apr 21, 2020

SergioRAgostinho commented Apr 21, 2020 • edited Loading

SunBlack commented Apr 21, 2020 • edited Loading

SergioRAgostinho commented Apr 21, 2020

SunBlack commented Apr 21, 2020

kunaltyagi commented Apr 21, 2020

SunBlack commented Apr 21, 2020 • edited Loading

SunBlack commented Apr 11, 2020 •

edited

Loading

kunaltyagi commented Apr 13, 2020 •

edited

Loading

SunBlack commented Apr 13, 2020 •

edited

Loading

SunBlack commented Apr 15, 2020 •

edited

Loading

SunBlack commented Apr 21, 2020 •

edited

Loading

SergioRAgostinho commented Apr 21, 2020 •

edited

Loading

SunBlack commented Apr 21, 2020 •

edited

Loading

SunBlack commented Apr 21, 2020 •

edited

Loading