RFC: clang-format GCS

[tracernz]

Rather than fighting issues with formatting in PRs etc. let's just get the pain out of the way and clang-format the whole thing in one massive conflicts-with-everything pull. And also a pre-commit hook to check it so we don't regress. Looks like Chromium have been doing this in most of their code for some time and are currently rolling it across the whole repo https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/7Zm1Z49TV7U/discussion.

[mlyle]

Let's land glowtape's stuff when it's ready, and then do it once (no precommit-checks, yet) as a first step. Can get fancier with precommit checks etc (need to be sure we have a good process for both frequent and sporadic developers to get "clean" / commitable).

We can't regress too badly in the next few months while we figure it out (most of the truly awful formatting is very old).

[mlyle]

Command would be something like:

 9627  find plugins app -name *.cpp -o -name *.h -print0 | xargs -0 fromdos
 9630  find plugins app -name *.cpp -o -name *.h -print0 | xargs -0 clang-format-3.9 -i

I've put together a crude .clang-format, which results in:

393 files changed, 20108 insertions(+), 20199 deletions(-)

[tracernz]

We should figure out what settings Qt Creator uses and match that, thus making it simple for everybody. Without a pre-commit check there will be nothing stopping it regressing.

[mlyle]

Without a pre-commit check there will be nothing stopping it regressing.

That's true, but we'll take the biggest conflict hit (and get the biggest benefit) the first time. I'm risk adverse and like taking baby steps :P

Also for the same big "conflict with everything" increment-- remove executable bits from tree files.

[mlyle]

find matlab make flight artwork branding python shared ground androidgcs -executable -type f \( -name '*.JPG' -o -name '*.jpg' -o -name '*.3ds' -o -name '*.c' -o -name '*.cpp' -o -name '*.h' -o -name '*.svg' -o -name '*.wav' -o -name '*.py' -o -name '*.txt' -o -name '*.m' -o -name '*.PNG' \) -print0 | xargs -0 chmod 644

[mkroehnert]

Hi, I want to share an insight from a robot framework I contribute to (https://gitlab.com/ArmarX/armarx).

The project uses a CMake-based build system and to keep the formatting of the source files clean in this project we added a specific target which runs a code formatter on all files which are marked as modified in Git. This target is executed automatically before any source files are compiled and thus removes the burden to rember running the formatter. So far, we did not face any issues with this approach. The only exception being that the used formatter (astyle) sometimes decides to format specific corner cases differently on separate runs. Of course, this behavior can be disable if absolutely necessary.

In order to start with a clean state, the formatter should be run once before activating such a mechanism, which unfortunately messes with the Git history.

[mlyle]

@mkroehnert that's a good idea / way to go at first.

@tracernz -- what do you think of, as a first-step, something like that? Best-effort-- only runs if you have the tool and aren't doing a release build. It should slow down the rate of us accumulating regression in formatting significantly. (I prefer things like this to precommit checks).

What I think I care most about is not losing your doxygen work and getting some kind of mechanism in place for that precommit. Reformatting can always be done again/later (even though it's yucky, it's always an option and always fairly mechanized).

[mkroehnert]

@mlyle if you are interested in how we did it, I can provide you with more details or the links to the relevant parts of the build system.

In addition to what I said above, we did not want to use pre-commit hooks. The problem with those hooks is, that they can not be activated by default. Each contributor would be required to install them or run a specific script to set them up.

[mlyle]

First piece done (initial reformat, styling file) in #1712. Next step, I think, would be to have makefile ability to force reformat, like we do with uncrustify.

[tracernz]

For checking code formatting, a Travis-CI job that runs clang-format on the diff and reports a commit status to Github might be a good solution. The failed status would be treated as a "use caution" when merging rather than an error (it's a shame Github status API doesn't allow any differentiation).

The problem with the Doxygen stuff is that the main source of regression is bad grouping etc. which isn't something readily checked by a machine.

[mlyle]

Running it on the diff-- something like "does this PR's tree have more lines of diff-from-reference-formatting than its base?" (not next, etc)?

Running on the files touched IMO will quickly degrade because you can't touch a file without completely fixing any formatting regressions.

I think before taking any more steps, we need to make it easy for end-users to run the formatter (and perhaps provide a reference pre-commit hook that does the formatting).

[tracernz]

One thing I liked about the Qt Creator plugin was that you can set it to reformat on save. Agree that any automated checking should only check diff. I have a tab open on my desktop machine with git/clang magic to do just that.