Add a project-wise `.clang-format` file

[diegoferigo]

Desired behavior

I'm opening this issue after a quick comment left in https://github.com/ignitionrobotics/ign-gazebo/pull/876#discussion_r668252576 where the usage of clang-format was briefly discussed.

Now it's a few years I'm interfacing from downstream with your code, and I see a pretty big effort to softly enforce an uniform style. There is a cppcheck running that catches something, but often I see developers leaving a considerable amount of comments in PRs just to fix the style.

The desiderata would be a uniform style that is enforced in CI level, and this is possible using tools like clang-format. There are other options, but it seems to me that this tool is becoming the standard for this task.

Implementation suggestion

A good start is first agreeing on a style that makes everyone happy (often not an easy one) and then adding a .clang-format file in the top-level folder of the repository. This file, then is picked up automatically when clang-format is executed from any folder of the same level or below. Particularly, in order to reformat a file, the following can be executed:

clang-format --fallback-style=none --style=file src/Server.cc

The formatted file will be printed in stdout or, if -i is passed, edited in-place.

Then, this process can be also automated in CI for PRs either by using an existing github action, or developing a new one. In this case, the option --Werror could be useful.

Many IDEs nowadays support ootb clang-format. I can confirm for QtCreator through one of the default plugins (it might need enabling it manually). I want also to highlight that coding with an IDE that automatically reformat the file every time is saved is a blessing, it makes coding much more pleasant (e.g. oh I forgot ;, let's add it in the current position in a different line and press save to automatically move it few lines earlier where it should belong).

Additional context

In https://github.com/ignitionrobotics/ign-gazebo/pull/876#discussion_r668276754, @adlarkin mentioned some early experiments that seemed to be fairly slow. I'm not sure which tool was used, maybe not plain clang-format since in my experience it works almost instantaneously.

[chapulina]

I see developers leaving a considerable amount of comments in PRs just to fix the style.

Yes! We need to fix that!

some early experiments that seemed to be fairly slow

I believe this is the branch I was experimenting on: https://github.com/ignitionrobotics/ign-gazebo/compare/ign-gazebo3...clang_tidy_error. It was using clang-tidy, not clang-format. Here's an old BitBucket PR trying to integrate it:

Wow this takes 26 mins on pipelines, as opposed to the usual 9 mins. 

In my memory it was hours for some reason :sweat_smile: I don't think it scales linearly with the length of CI, but nowadays GitHub Actions takes ~1 hour instead of 9 mins, so I'd expect more than a 17 min increase in CI time. One option would be to run it in a separate GitHub action so it runs in parallel with the rest of CI.

clang-format

I believe @mjcarroll and/or @azeey had local clang-format setups but I believe we never got it to play nicely with our style.

A good start is first agreeing on a style that makes everyone happy

We've been following the same style as Gazebo classic. There are various aspects of this style that are tough to enforce with linters though, that's why we've been kind of stuck.

Updating our style to make it easier to enforce is a possibility, but it's also a huge effort that we shouldn't commit to unless we're sure we can put the infrastructure in place, as well as the time to migrate to a new style.

[mjcarroll]

I believe @mjcarroll and/or @azeey had local clang-format setups but I believe we never got it to play nicely with our style.

At the last time I looked, there was no way to construct a clang-format that also satisfied the gazebo style guide.

[azeey]

The issues I faced with clang-format were:

• Braces on new lines for lambdas, which I think has been fixed since (https://reviews.llvm.org/D44609)
• access modifiers (public,private, protected) on the same line as members. I've had some luck with modifying clang-format to do this, but that requires building it from source. I'm also not confident in the implementation to create a PR upstream.

[diegoferigo]

some early experiments that seemed to be fairly slow

I believe this is the branch I was experimenting on: ign-gazebo3...clang_tidy_error. It was using clang-tidy, not clang-format. Here's an old BitBucket PR trying to integrate it: [...] In my memory it was hours for some reason sweat_smile I don't think it scales linearly with the length of CI, but nowadays GitHub Actions takes ~1 hour instead of 9 mins, so I'd expect more than a 17 min increase in CI time. One option would be to run it in a separate GitHub action so it runs in parallel with the rest of CI.

This was my suspicion when I read that it was slow. clang-tidy is comparable to cppcheck, both apply a static analysis of the code that requires working on some IR, and it's much more expensive than plain clang-format that just processes file-by-file I guess by parsing sequentially the lines.

Updating our style to make it easier to enforce is a possibility, but it's also a huge effort that we shouldn't commit to unless we're sure we can put the infrastructure in place, as well as the time to migrate to a new style.

Well, if the aim is just enforcing the style, the simplest implementation I can make on the fly could be a CI check as simple as:

find src/ include/ -name "*.hh" -or -name "*.cc" -exec clang-format --style=<whatever> -i {} +
test $(git status --porcelain | wc -l) -eq 0

A good start is first agreeing on a style that makes everyone happy

We've been following the same style as Gazebo classic. There are various aspects of this style that are tough to enforce with linters though, that's why we've been kind of stuck.

I see the problems. However, even if a carefully tailored .clang-format file would not be 100% compatible with the existing codebase, maybe accepting a small compromise and introduce a minor style change could still be worth if it means saving the effort of processing each PR by visual inspection. Beyond clear upstream advantages, it's also time saved in downstream, because we don't have to mind of remembering all the small style details (frankly, it rarely happens) and personally it was the first time I read the style documentation. I always tried to be as compliant as I could just considering the code I see around, but practically I always do mistakes.

• access modifiers (public,private, protected) on the same line as members. I've had some luck with modifying clang-format to do this, but that requires building it from source. I'm also not confident in the implementation to create a PR upstream.

Yes you are right, this is tricky, it is not supported yet. It seems the only compromise at the moment. The best thing you can get is using AccessModifierOffset: 0 obtaining:

class MyClass
{
    private:
    int foo;

    protected:
    bool bar;
}

[vatanaksoytezer]

Updating our style to make it easier to enforce is a possibility, but it's also a huge effort that we shouldn't commit to unless we're sure we can put the infrastructure in place, as well as the time to migrate to a new style.

I have some free time this summer, and want to involve more with Ignition project. I've always enjoyed infrastructure stuff so I can do all the migration for you if you agree on a new style.

From personal experience I've always been almost intimidated by styling comments and hard to enforce when you don't have a linter. In ros-planning projects we started to use pre-commit with Github Actions and have gained significant amount of time because we don't check styling anymore. You can check moveit2 on how we use clang-tidy and clang-format with pre-commit.

[chapulina]

Thanks for the help, @vatanaksoytezer !

I think the next step would be for someone to propose a clang-format configuration that minimizes the changes we'll need to make. It would also be nice to integrate clang-tidy in a separate GitHub Action so it can run in parallel to our current CI.

[diegoferigo]

FYI the enforcing of a project-wise clang-format is something that we are also interested in our projects, and we always postponed it. We relied to a visual inspection of PRs and ask contributors to fix the style if we were catching a mismatch. This is error prone, especially when multiple maintainers can merge PRs, something could slip and be merged unformatted in master

To address this problem, I created a simple action stored in diegoferigo/gh-action-clang-format that runs clang-format on all sources. This is still experimental, but it could provide a starting point in case you either want to adopt it in CI, or vendor its functionalities in an internal script. See https://github.com/robotology/gym-ignition/pull/375 for a downstream example.