Create a clang-format config for PCL style.

[JamesGiller]

The clang-format config enforces spacing and line-break guidelines from
the PCL style guide, but not guidelines such as all cap constants or
parenthesised return values etc.

Unfortunately, clang-format has limitations that mean a couple of the
style guidelines cannot be enforced, specifically:

1. Members affected by access qualifiers cannot be indented by one more level
2. Return type of functions will go on own separate line, not even the
   same line as template parameters as in one example from
   section 2.3 of the style guide

resolves #2106

[taketwo]

Hi, thanks, I'll give it a try. Which version of clang-format are you using?

Members affected by access qualifiers cannot be indented by one more level

So formatting existing code will result in enormous diffs. That's really bad :(

[JamesGiller]

I used clang-format-5.0.

For the guidelines that aren't related to spacing (e.g. parenthesised return values) would the maintainers be interested in using clang-tidy?

[SergioRAgostinho]

So formatting existing code will result in enormous diffs. That's really bad :(

Let me incept this idea: at this point I believe it is more important to have an automated tool to format code and to do formatting checks on the CI than preserving the current code style. If we are happy with something, I would simply reformat all files in a single commit. We do something really bad (hopefully) once to end this issue once and for all. This of course, assuming we exaust all our options with the existing formatting tools.

For the guidelines that aren't related to spacing (e.g. parenthesised return values) would the maintainers be interested in using clang-tidy?

After reading this I would say yes.

[taketwo]

After reading this I would say yes.

That's interesting. But then shouldn't we rather drop this guideline? Anyway it wasn't consistently enforced.

[SergioRAgostinho]

But then shouldn't we rather drop this guideline? Anyway it wasn't consistently enforced.

I personally didn't know about it and never enforced it. I'm ok with getting rid of it.

I like the clang-tidy/clang-format combo and I would like to adopt it even we have to let go some guidelines.

[JamesGiller]

There's a list of checks available in clang-tidy here. It's also possible to add new custom checks. How should the checks be chosen?

[SergioRAgostinho]

Thanks for the reference. I think it's better to only start fiddling with clang-tidy once we've agreed on formatting first. This is something we will need to escalate to our "elders council". I also think Sergey has mixed feelings about my proposal, since the downsides are considerable, which normally indicates its good to bring older maintainers into the discussion. I'll write to them today.

Edit: Email sent.

[SergioRAgostinho]

I've been discussing your findings and I have a couple of news. I believe we're going to drop the two points mentioned that you're unable to implement with clang-format, so ignore those for now.

Can you find out if it is possible to only enforce formatting on changes i.e., if a user submits a PR with some changes, is it possible only to enforce formatting on that diff?

From our side it is important to provide a way for our users to format their code prior to committing the changes. We can accomplish that by providing them with a method to do so. Optionally we can also provide a git hook to that effect.

The second important point is to have the same formatting check running on Travis CI, so that we immediately flag PRs which are not compliant.

Can you find out if this is possible?

[taketwo]

I agree we should postpone clang-tidy discussion until after formatting issues are solved. (For the record, I've made an attempt at integrating clang-tidy into CI of one other project that I participate in maintaining, but that came with it's own set of challenges and for the time being I abandoned that idea.)

Can you find out if it is possible to only enforce formatting on changes i.e., if a user submits a PR with some changes, is it possible only to enforce formatting on that diff?

Yes, it's possible to format diff with respect to the master, see example script here.

From our side it is important to provide a way for our users to format their code prior to committing the changes.

This, by the way, means that we have to reformat the whole repository. Because, otherwise, how would we explain users that they need to run clang-format only on the lines that they touched?

[SergioRAgostinho]

This, by the way, means that we have to reformat the whole repository.

I have an intuition why this might be necessary but I'm sure I'm not fully understanding the reasons behind this requirement. Anyway I'm totally pro reformatting the entire repo so I'm happy with it anyway 😄

Because, otherwise, how would we explain users that they need to run clang-format only on the lines that they touched?

We would need to the append an explanation of the process to CONTRIBUTING.md detailing how to generate the diff and applying clang-format-diff on top of it. Ideally, we really supply users with a pre-commit git hook which automatically enforces the formatting changes the before the commit takes place. The job for the users would just be to install clang-format and python/clang-format-diff if he/she wants the automatic formatting feature. Although writing a cross platform script... well python again.

I have the impression this should be possible, albeit harder, in case we really want to do it.

[taketwo]

Regarding using git commit hooks for formatting. I've had an experience with that before and I think it's a mess. There are two problems. First, the hook script has to be extremely robust, because if it fails, then the user is not able to commit. And if it does something wrong (without explicitly failing), then it's even worse because some gibberish will be committed. Second, even if you put the hook script inside the repository, it won't be automatically set up in a cloned repo, a manual step on the user's side is needed to install it.

Now imagine we get a new pull request that is not formatted. I think it's easy to just say: "please run clang-format over the files, style definition is included in our repository". Compare with: "setup a hook, uncommmit your commits and recommit them again such that the hook will (hopefully) take care of formatting".

The job for the users would just be to install clang-format

Speaking of this, it's not "just". As I pointed out some time ago, different versions of clang-format produce different results! The user has to install exactly the same version as on CI, otherwise there might be situations when locally everything is formated, but CI fails.

From our side it is important to provide a way for our users to format their code prior to committing the changes.

This, by the way, means that we have to reformat the whole repository.

I have an intuition why this might be necessary but I'm sure I'm not fully understanding the reasons behind this requirement.

Think about it this way. We add official .clang-format file to our repository and modify style guide to match the behavior of clang-format. What will users expect? That the code should be formatted with this style. Many users will have their IDE setup to automatically pick up .clang-format and apply formatting to changed files on save. People will just commit formatting changes along with their contributions thinking this is a right thing to do. Then they will submit PRs and we will tell them: "You see, even though we have this style definition, you shouldn't apply it. You only need to run some obscure script to format only diffs before committing. You now go back and separate style changes from actual changes and recommit and resubmit your PR." How frustrating is that, for both sides?

Finally, in the e-mails you invoked the argument that reformatting the whole repo will lead to the need to rebase old PRs. Well, fine, that's another necessary evil. In most cases it won't be a significant effort anyway. And, besides, how many of the old PRs do we expect to merge (realistically)?

[SergioRAgostinho]

Hmm. So:

• Ease of explanation to the users.
• IDEs picking up on the format file and enforcing the style globally anyway.
• Overestimation of the effort involved in rebasing the existing contributions and in the predicted number of PRs that will be merged. (Touché on this last one 🤺 )

Plus I'm also sensing you're sure that global is the way to go. I have doubts but I personally also look forward to adopt the style globally at once. I say let's go with that.

I suggest we proceed with CI integration in this PR and then finalize with the commit which reformats the entire repo.

@JamesGiller care to continue with the task and start working on the necessary changes on Travis CI to check for the current style? My idea would be to run the format check as the first CI stage. If formatting fails we don't even bother building.

Regarding the clang-format version I would suggest adopt to the most recent release.

[JamesGiller]

@SergioRAgostinho, OK! Just to clarify, by changes to the Travis CI you are referring to something like this script that @taketwo linked above?

[SergioRAgostinho]

I'm not sure if you closed this intentionally but my idea was to add commits to this PR.

Just to clarify, by changes to the Travis CI you are referring to something like this script that @taketwo linked above?

pcl/.travis.yml

Lines 59 to 86 in f6a443e

	jobs:
	include:
	- stage: Core Build
	env: TASK="build"
	compiler: gcc
	script: bash .travis.sh $TASK
	- env: TASK="build"
	compiler: clang
	script: bash .travis.sh $TASK
	- stage: Extended Build and Tests
	compiler: clang
	env: TASK="build-examples"
	script: bash .travis.sh $TASK
	- compiler: clang
	env: TASK="build-tools"
	script: bash .travis.sh $TASK
	- compiler: clang
	env: TASK="build-apps"
	script: bash .travis.sh $TASK
	- compiler: gcc
	env: TASK="doc"
	script: bash .travis.sh $TASK
	- compiler: gcc
	env: TASK="test-core"
	script: bash .travis.sh $TASK
	- compiler: gcc
	env: TASK="test-ext"
	script: bash .travis.sh $TASK

You'll need to add a new stage here, which will run before everything.

The corresponding shell code will need to be written on travis.sh. It's something like that script Sergey linked but it should be much simpler since you need to run the tool on all C++ source files instead of simply newly committed changes.

The idea is to run clang-format with the format file you implemented on the entire repo. clang-format will likely return an error code >0 if it finds format mistakes and that is enough for Travis to identify that the stage failed.

[JamesGiller]

I think I accidentally clicked the "close" button but didn't realise. I will continue with this task.

[SergioRAgostinho]

@taketwo How about also giving a try at cmake_format for cmake files?

[taketwo]

clang-format will likely return an error code >0 if it finds format mistakes and that is enough for Travis to identify that the stage failed.

I think it will be extremely useful to also print out the formatting mistakes found, maybe even directly in a diff format that can be applied on the users side.

@taketwo How about also giving a try at cmake_format for cmake files?

Thanks for the link, I did not know about this tool. Generally, yes, I'd be pro formatting CMake files as well, but I wouldn't tie it with code formatting and this PR. I'd like to get more familiar with the tool first, maybe try to use it with my private projects to get a feeling how stable/reliable it is, is the output formatting good, etc.

[JamesGiller]

I think it will be extremely useful to also print out the formatting mistakes found, maybe even directly in a diff format that can be applied on the users side.

There is a repo run_clang_format that seems to tick all the boxes.

If it's fine to just give a yes/no answer to "does the code satisfy the format rules?" then I have something worked out that I will push for you to check out.

[taketwo]

If it's fine to just give a yes/no answer to "does the code satisfy the format rules?"

The more I think about it, the more I'm convinced that this won't do. As was established before, different clang versions format slightly differently. Imagine a contributor has Clang 3.x and submits a formatted patch. CI runs with the latest Clang and maybe finds some very subtle formatting difference. And thus yells back that something is mis-formatted. What should we do then? It's likely that the formatting "problem" is too subtle to even notice. So somebody (either a maintainer or the contributor) will have to install the latest Clang and run formatting to find out what's wrong. I don't think this is a good process.

[JamesGiller]

@taketwo The run_clang_format repo I mentioned seems to be able to produce a diff. The contributor would have to view the Travis build in order to copy the diff into a file or otherwise apply it.

Is the preferred way to proceed to check the whole repo for formatting errors, fail the build if there are any and print a diff to apply in order to fix them?
I know that Travis could automatically format the code before deploying it, but is it also possible to format before updating the GitHub repo? I think if it is possible it might involve creating an extra commit on behalf of the contributor.

[SergioRAgostinho]

Regarding the clang formatter python wrapper tool. It looks cool 👍 .

Is the preferred way to proceed to check the whole repo for formatting errors, fail the build if there are any and print a diff to apply in order to fix them?

Too my understanding, yes.

I know that Travis could automatically format the code before deploying it, but is it also possible to format before updating the GitHub repo? I think if it is possible it might involve creating an extra commit on behalf of the contributor.

It might be, but it also involved having a bogus user with write permission to push the new commit automatically to the user's forks. I'm not comfortable with this.

[SergioRAgostinho]

Given the new CI, I thought about jump starting this one again. So currently missing:

• move the azure pipelines. Now it has more than spare time to run the formatting.
• fetch clang format from an official source or fork into PointCloudLibrary org.

[taketwo]

I think we should have a Docker image with the "formatting toolbox" installed. That would include clang-format (of a certain "golden standard" version), the helper script we talked about before. In future this can also include spell checker, linter, clang-tidy and whatnot.

[SergioRAgostinho]

Separate image for these assistive tools or merge it into the current one you created? I'm fine with both actually.

@JamesGiller give us a ping in case you want to carry on this final push. I'm saying this because you seem to be inactive on GitHub for some time now.

I might personally take a look at this over the weekend.

[taketwo]

Separate image for these assistive tools or merge it into the current one you created? I'm fine with both actually.

Separate, there is no reason to tie together it with the build environment. We may use a completely different distro as a base, e.g. Alpine Linux which is lightweight.

I might personally take a look at this over the weekend.

I can put this on my list as well, though I won't have time this weekend.

[SergioRAgostinho]

I can put this on my list as well, though I won't have time this weekend.

Probably better. I have 0 Docker experience.

[JamesGiller]

Hi guys,
Sorry, I returned to my home country recently and totally lost track of this issue. I would be happy to continue to work on it now, although I think I would need some help from more experienced contributors since the proposed final changes seem to involve deeper integration of new tools into the project infrastructure.

[SergioRAgostinho]

Don't worry about it James. Your contribution for this one was more than we could hope for. It's just easier for Sergey to complete this final stretch. Whenever you feel like lending a hand again, please do, and if you need ideas ping us.

[jasjuang]

+1 for clang-format, this will make the code so much easier to read!

[stale]

This pull request has been automatically marked as stale because it hasn't had
any activity in the past 60 days. Commenting or adding a new commit to the
pull request will revert this.

Come back whenever you have time. We look forward to your contribution.

[kunaltyagi]

Has there been any development regarding this?

Going by the discussions, we need:

• Config files:
  • .clang-format
  • .cmake-format.yaml
  • Others?
• docker image with latest versions of tools installed (unless one of them has settings against the style guidelines):
  • Lightweight base
  • clang-format
  • cmake-format
  • clang-tidy (though once all files are formatted, it doesn't make sense to have both clang-format as well as clang-tidy around)
  • helper scripts
• helper scripts:
  • compare diff for c++ and cmake files
  • anything else?

[SergioRAgostinho]

No updates. From the listing you did, there's still a lot left to do so I would just focus on having a viable image to run clang-format. Small increments so we can at least have some progress. Above all we need to figure how long does it take to run a full check in Azures Pipelines.

[kunaltyagi]

Is there a reason for recommending Alpine as base? It'd be easier to use ubuntu:18.04 or debian:latest (or fedora) as base image to get access to the packages. clang-format for example isn't available on alpine. It'd need to be compiled and installed. I've gotten PCL compiling in ubuntu:18.04 along with gcc-9, clang-9, clang-format, clang-tidy.

[taketwo]

Is there a reason for recommending Alpine as base?

I use Alpine in some of my projects for this purposes. The reason to prefer it is that it's lightweight and generally more up-to-date than Ubuntu.

clang-format for example isn't available on alpine

It is. Here is a Dockerfile that will get you an image with clang-format:

FROM alpine:edge
RUN apk add --no-cache clang bash

[kunaltyagi]

apk add clang

I thought this would add only clang and llvm as it does on other systems, not clang-format. Got it now. Will go ahead

[kunaltyagi]

Assuming we have the style to what we want (via a commit), should I create

• a shell script to run clang-format
  • easier, hackier. Can be extended easily
  • can be integrated in cmake separately to achieve the same results but still be light weight (in terms of docker)
• integrate clang-format in cmake
  • might integrate with development workflow better since compilation would trigger clang-format first making code automatically compiling and formatted before commit
  • might work nicely with the build pipelines being used
  • difficult to extend easily unless by creating an external script 😄

We'll anyways need scripts to check git diff post clang-format

[SergioRAgostinho]

From the users perspective I like the idea of providing a cmake target called format. That way, if we need to provide instructions we have a quick way of telling users what to do.

• Install clang-format
• run make format

No additional knowledge needed. I'm just wondering how this will propagate to the IDEs.

[jasjuang]

Here is a way to do make format

file(GLOB_RECURSE ALL_FILES *.cpp *.h *.hpp *.cu *.cuh)

set(CLANG_FORMAT_EXECUTABLE "clang-format" CACHE STRING "clang-format")

add_custom_target(format COMMAND ${CLANG_FORMAT_EXECUTABLE} -i ${ALL_FILES})

[SergioRAgostinho]

@ kunaltyagi I forgot to add: don't spend too much time thinking how to implement pre/post commit checks at this stage. Let's just get the foundations in place:

1. A manual target we can invoke
2. A format check on Azures Pipelines.

[taketwo]

Closing this in favor of #3188. @JamesGiller thanks for your work.