Add format compilation target that applies clang-format to whitelisted modules

[kunaltyagi]

• .clang-format file added from details gleaned via website. PTAL
• Added cmake file to keep clang-format detection an target addition separate
• Changed main CMakeLists.txt to include said cmake file

TODO:
Compile and test on:

• ubuntu:18.04 + make
• ubuntu:18.04 + ninja (Not a blocker. Makes incremental builds faster, and I like it)
• alpine:edge (CMake config currently failing)
• any others?

Please run cmake and check if make format is correct as per the standards.

It modifies all the files, including doc, tools, test, outofcore, examples, .... */3rd_party so that might not be the desired behavior or whatever reason. Though we do own all code in the repo and it should be ok to format it as per the guidelines.

Executive summary

This PR adds:

• A new clang-format config decided upon in Create a clang-format config #3190. This config defines the new PCL style guide. The old document with "verbal" description of the style is thus obsoleted and has to be updated.
• A shell script that formats PCL codebase using clang-format. This is not intended to be used by developers directly.
• A make target format that formats PCL codebase. This is to be used by developers.

At the moment formatting will only be applied to a selected list of PCL modules. This while listing will be removed after the "testing period" is over.

Closes #3190, #2106, and #2376 (obsoleted).

[taketwo]

One thing to consider: different versions of clang-format (with the same style file) may format the code differently. Therefore, if we don't mandate a particular version of the formatter (the same as on CI), then the users of make format might end up with a) sizeable formatting diffs; b) CI format check failures.

(Note that I use the verb may. From my past experience some versions were fully compatible (or at least did not expose incompatibilities on my projects), whereas some were not. We will need to check empirically I'm afraid.)

I guess the only way out is to make the "official" formatter version(s) an explicit part of the style definition. CMake thus has to check if the version that the user has is compatible.

[kunaltyagi]

different versions of clang-format (with the same style file) may format the code differently

So, I decided to check it out. For future reference, I'm detailing the broad changes between different versions of clang-format since I didn't find them after a cursory search.

• clang-format-3.9: baseline
• clang-format-4.0: removes duplicate #include too. Adds spaces after inline comments
• clang-format-5.0: proper indentation for multi line doxygen comments
• clang-format-6.0: adds namespace <name> comment at the end of namespace block
• clang-format-7.0: no changes
• clang-format-8.0: no changes
• clang-format-9.0: no changes

This means we can choose any version after 6 based on the default version available on the systems we are targeting preferentially. I leave the final choice to you.

And yes, it does make sense to say: this .clang-format file with clang-format-X.Y is the standard, until it's changed. We don't know if clang-format-10.0 will introduce large diffs, but IMO it seems unlikely.

[SergioRAgostinho]

Given your discoveries I would keep the requirement loose: version >= 6.0, until we're required to change it again.

[taketwo]

Wow, thanks for the research. I agree with @SergioRAgostinho, let's require >= 6.0.

Another question you've raised is which files we should format. I think we should exclude doc/ for now since it's not so trivial to format. It includes both source files and RST tutorials which sometimes include verbatim lines from the source files. Formatting them will change line numbers and we'll need to go through a very labor-intensive procedure of correcting them in RST files. I'd certainly leave this for future.

[kunaltyagi]

PTAL. Should work on alpine, ubuntu both.

[taketwo]

Let's stay with 3.5, we don't want to drop support for Ubuntu 16.04 yet. PCL has an utility function to filter a list by regex.

[kunaltyagi]

Let's stay with 3.5, we don't want to drop support for Ubuntu 16.04 yet.

OK. Got it. I need to exclude a regex, so the utility function doesn't help but steers me towards the right direction. Thanks

[kunaltyagi]

I've made a bunch of changes as suggested. I'll try to remove any errors tomorrow. Just pushed a trial version.

[taketwo]

A meta-comment: please don't squash/force-push without need. It makes incremental reviewing harder since it's not clear what has been changed in between now and the last time I reviewed. When we are done with preparing the PR we will have an opportunity to reshape the history to our liking before merging.

[kunaltyagi]

@taketwo @SergioRAgostinho PTAL. make format works on Mac OS 10.14, Ubuntu 14.01, 18.04. Flann error persists on alpine (haven't devoted time on it). Ninja error is currently out of my understanding based on the issue discussion

Test matrix:

clang-format	3.9	6	8
ubuntu 16.04	✅	✅
ubuntu 18.04		✅	✅
alpine (cmake config steps)			✅
Mac OS 10.13.6			❌
Mac OS 10.14.5		✅	✅

Sorry about the rebase+force before.

[taketwo]

Since my comments got lost, I will re-add them again.

[taketwo]

Great, thanks for applying the changes. I think the scripts are in a pretty good shape now!

Let's discuss the next steps. This format target is of no use until the repository is reformatted to the style. Question is when we are going to do this. Before/after release? We have a bunch of pending PRs which we probably don't want to screw up with reformatting.

One idea that I had is to introduce formatting gradually. That is, pick a module that currently has no pending changes and add it to the formatting "white-list". Format it. Then we can start experimenting with CI job that checks formatting (but only in that module). Such an approach will allow us to move forward and try things without an enormous reformatting upfront. What do you guys think?

[kunaltyagi]

We have a bunch of pending PRs which we probably don't want to screw up with reformatting.

How about any future PR needs to be formatted correctly? By creating a separate branch called formatted, it can be ensured that the PR will be integrated correctly. Old PR get merged, master gets formatted and pushed to formatted. New PR compare against formatted

That is, pick a module that currently has no pending changes and add it to the formatting "white-list"

Maybe I can modify the script to add multiple targets. format-all, format-doc, format-segmentation, etc. Then this would be much easier. I'm already doing the regex check for doc, it'll be eliminated completely by doing this. Either we can list the modules by hand, or some PCL utils (I do see in CMake output the different modules referenced separately) or discover them

Then we can start experimenting with CI job that checks formatting (but only in that module)

How to do this should be the focus. If this is not automated and done in parts, it'll be an uphill battle.

[claudiofantacci]

@claudiofantacci sheds a tear every time someone says that. :)

I believe the trend now is to either use full Visual Studio for the full IDE, or Visual Studio Code if just the editor is required. Both probably, offer the same formatting functionality. @claudiofantacci Are we missing any obscure corner cases here?

Well if you are on Windows you just use/need Visual Studio and then you may want to use Visual Studio Code. As you already find out, they both offer clang-fromat (although different version and starting from different release) so they offer simila formatting functionalities.

I'm stuck with VS 15.6.7, so I don't have clang-format 😭 However I'm trying to update to a newer version so let me know whether you need tests on my sude or not 👍

[taketwo]

I'd say let's move on with a shell script. We only have limited resources and a truly cross-platform solution would require too much effort. In future VS will certainly update to a newer clang-format and our Windows users will also be covered.

[kunaltyagi]

I've added a script (There's a spurious echo and an exit statement, which I'll remove after I'm done testing on ubuntu-16 and ubuntu-18). PTAL

[taketwo]

Could you elaborate what's the purpose of this block? The formatter executable is passed as $1 to the script, why don't we use it directly?

[kunaltyagi]

I added this despite the FindClangFormat.cmake because

• cmake will inform developers nicely about the missing version
• for just formatting, there's no need to run cmake. This script is self sufficient, allowing it to be run without installing dependencies required by cmake. Possible scenario can be CI/CD or a format based hot-fix to a PR from a machine with limited dev-tools

[taketwo]

What about passing PCL_DIR as an argument to the script? Then no need to cd/pwd and guessing.

[kunaltyagi]

2 separate reasons:

• reducing confusion and checking for mixing clang-format-path and pcl-source-path
• technically, the guess is very good except a corner case (soft-links along the dirname $0)

Only power users need to provide the PCL_DIR as input args or env var, and I figured the env var would be less confusing.

The change to cli arg input is easy I had that before I moved to env var based backup

[taketwo]

I understand your idea of making it possible to run format.sh without arguments, however I'm not convinced with the use-cases. You cited CI/CD and machines with limited dev tools. I think in the first case we are in control of the environment and it's trivial to supply arguments. The second case seems a bit of a stretch to be honest. If the machine does not have dev tools to build PCL, why would you even edit PCL code?

The reason why I'm sort of against these guesses is that it duplicates functionality and increases maintenance burden. We now have clang-format discovery and version checking logic implemented in two different languages and located in two different scripts. We will need to pay attention that these implementations stay coherent and spend double effort updating both.

[kunaltyagi]

No problem. I've made the changes and also added cuda. As per clang-format output, it should be okay in the future too.

[taketwo]

Great, thanks. One last thing (promise 😄) that I want to discuss is the location of the script.

Generally, at the top level of our directory hierarchy are modules (aka libraries: filters, kdtree, etc.). Four exceptions are cmake, doc and examples and test, though they are all essential parts of PCL, so deserve to be there. I see the formatting script as something auxiliary and thus rather belonging to a hidden directory. In fact, we already have a few helper scripts in .github, and yet another one in .ci/scripts. I was wondering if we should merge these and put format.sh alongside. Ideas? Create .dev/ in the root? Or everyone is happy not hidden dev/? Or scripts/?

[SergioRAgostinho]

I'm totally fine with .dev.

[taketwo]

So to finish this off I propose to add the final formatting config and regroup commits:

• add clang-format config
• add shell script
• add cmake target

[taketwo]

As expected, works well with Ninja.

[kunaltyagi]

For reference, closes #3190 and #2106

[taketwo]

I updated the PR description (i.e. the first comment) with a summary and links to the issues you mentioned (this will make sure that they are closed automatically once this one is merged).

[SergioRAgostinho]

LGTM. Thank you all. I will just give it a quick test and merge if everything goes well.