[Formatting] automatic C++ code formatting

[windelbouwman]

Hi,

Thank you for creating this project!

Would it be an option to apply something like clang-format to the C++ code? I saw that the python code is formatted already using ruff, but the C++ codebase has no autoformatter configuration file. Can this be added? It can also be enforced with a github action. This would help contributions to be in correct style.

[leducp]

Hi,

It is an option and we discussed a bit about this with @trns1997 . The point that we didn't find a proper setup yet because we didn't find a way to configure it in such a way that it does not disrupt the current sources.
For instance, I do want to keep alignment in one line switch case because I really feel that is is much easier to read like that.
Our conclusion was that it is useful and we should start slowly, adding rules step by step aaaaannnd, we lack manpower 😅

So yes, any contribution in this direction is welcomed, with the condition that the tool do not disrupted the current code base without reasons :)

[windelbouwman]

Do you have any suggestions on how to start introducing clang format? What would be a path / strategy?

[trns1997]

Well i had started with this #306 . But the clang-format is not super adaptable for cases like in this file https://github.com/leducp/KickCAT/blob/master/examples/master/common/include/CanOpenErrors.h where having it structured this way is way more readable but it does not comply with clang-format standards/configuration. So not really sure what path to take, maybe clang-format is not the tool we're looking for, or we might want to limit clang-format's scope to specific things like pointer/ref alignment and other such details. We're open to suggestions, we just want to avoid a massive reformat of the existing code base (unless justified).

[windelbouwman]

Okay, I did some experimenting. My strategy so far:

• Create a .clang-format file, with clang-format --dump-config > .clang-format to arrive at a file with all settings

Then, repeat these steps:

• Format protocol.cc with clang-format
• Check with git diff the changes
• Modify .clang-format accordingly, to arrive at no diff

The thing that I found was that clang-format cannot format this case arm:

case 0x0000:    { return "no error"; }

It will always make it like this:

 case 0x0000: {
                return "no error";
            }

However, it can format this style of case arm:

case 0x0000: return "no error";

Is this acceptable? I think clang-format cannot format the code as is, it does not have enough options to do so. Another option would be uncrustify, however it seems that clang-format is the currently popular and widely used formatter.

[trns1997]

Hmm this could be acceptable honestly. What do you think @leducp ?

[windelbouwman]

There are definitely more places where formatting will occur.

See my attempt here: #381

[leducp]

We need to keep the braces: this is a standard coding style in safety critical software which kickcat is derived from.
The rational: when the code evolve, the lack of braces tend to ease the introduction of bugs because the scope was unbounded to start with.

I wonder if we could let the autoformat skip the switch cases and works on the orther part because I remembered that this was one of the trickiest thing to do with it. It will still be really useful for the other parts of the code

[windelbouwman]

Okay, given this situation, my guess is that clang-format is unusable. uncrustify seems more flexible, however it is near impossible to configure. I found whatstyle, this is a python script that brute forces all possible settings of a formatter to try to find a set of settings matching the current source code. Maybe we could use that?

[trns1997]

@windelbouwman open to suggestions, go ahead and ping us we'll review with pleasure.

[windelbouwman]

I think uncrustify is the way to go. I added a bash script which runs uncrustify in the same way its done for the python code. See #381 .

[windelbouwman]

I used the command:

$ bash scripts/format_cpp.sh apply lib/src/ESI/Parser.cc

However, the uncrustify config file says that namespaces are indented, but the Parser.cc file does not do this. What is wise to do now? I think uncrustify is the way to go, but the code is not yet formatted consistently with the uncrustify config file.

[trns1997]

I think it's worth to maybe do a PR on a subset of files (maybe a folder) and see the result. We can review it all together and basically tweak the uncrustify config to converge to rules we like.