Create a linter tool to ensure source code formatting

[henrikssn]

While most source code roughly follows the guidelines, it has occured to me that there are many cases of inconsistent formatting across the code base. This is annoying, but good news is that it is easily automatable (so that we can focus on more important things, such as my fiber PR #439 :)

Coding convention is here: https://github.com/modm-io/modm/blob/develop/docs/coding_convention.md

This is an ask to either write a linter for the existing style (which we can run in CI), or to switch to a style where there already is a supported linter / formatter tool (my proposal would be Google style, but I am obviously biased).

See https://google.github.io/styleguide/cppguide.html and https://github.com/cpplint/cpplint.

Any opinions here?

[salkinium]

Yes please!

I'd prefer adapt our coding style to the closest that clang-format/cpplint can support. Definitely not interested in maintaining yet another custom tool.

We've refrained from massive reformatting during porting/refactoring xpcc to modm, since it touches every file and thus breaks the git history, and we needed to extract the copyright authors to relicense modm.
I'm still not super happy about rewriting every file in modm just to get formatting done, so maybe we could only run the linter on files that were added or changed, otherwise there's too much noise?

[henrikssn]

From http://clang.llvm.org/docs/ClangFormat.html, it seems we have the following code styles to choose between:

• Google: https://google.github.io/styleguide/cppguide.html
• LLVM: https://llvm.org/docs/CodingStandards.html
• Chromium (basically same as Google): https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md
• Mozilla: https://firefox-source-docs.mozilla.org/code-quality/coding-style/coding_style_cpp.html
• WebKit: https://webkit.org/code-style-guidelines

I think it is a good idea to only require changed files to adhere to the format. In a few years, we can reformat the rest which haven't seen any changes so far. Should not be too hard, something like git diff --name-only should give us the list of changed files in a PR.

Here is a starting point which uses GitHub Actions: https://github.com/cvra/clang-format-action

[henrikssn]

To kick this off, I tried the webkit style on my fiber PR. See the result here: henrikssn@328dbb2

I also tried Google style (which I would prefer): henrikssn@2e3c1d1

Any opinions?

[salkinium]

We'll need our own custom clang-format style that is as close as possible to what we have, otherwise we're constantly reformatting files in PRs.
I'll play around with https://zed0.co.uk/clang-format-configurator/ and see what I get.

[salkinium]

This is what I got from pasting in a couple of files from modm and poking around the settings until it looked familiar to me again:

modm's clang-format style

BasedOnStyle: Microsoft
AccessModifierOffset: '-4'
AlignAfterOpenBracket: Align
AlignConsecutiveMacros: 'true'
AlignConsecutiveAssignments: 'false'
AlignConsecutiveDeclarations: 'false'
AlignEscapedNewlines: Left
AlignOperands: 'true'
AlignTrailingComments: 'true'
AllowAllArgumentsOnNextLine: 'true'
AllowAllConstructorInitializersOnNextLine: 'true'
AllowAllParametersOfDeclarationOnNextLine: 'true'
AllowShortBlocksOnASingleLine: 'true'
AllowShortCaseLabelsOnASingleLine: 'true'
AllowShortFunctionsOnASingleLine: Inline
AllowShortLambdasOnASingleLine: Inline
AllowShortLoopsOnASingleLine: 'false'
AlwaysBreakAfterReturnType: None
AlwaysBreakBeforeMultilineStrings: 'false'
AlwaysBreakTemplateDeclarations: 'Yes'
BinPackArguments: 'true'
BinPackParameters: 'true'
BreakAfterJavaFieldAnnotations: 'false'
BreakBeforeBinaryOperators: None
BreakBeforeTernaryOperators: 'false'
BreakConstructorInitializers: BeforeColon
BreakInheritanceList: BeforeColon
BreakStringLiterals: 'false'
ColumnLimit: '120'
CompactNamespaces: 'false'
ConstructorInitializerAllOnOneLineOrOnePerLine: 'false'
ConstructorInitializerIndentWidth: '4'
ContinuationIndentWidth: '4'
Cpp11BracedListStyle: 'true'
DerivePointerAlignment: 'false'
DisableFormat: 'false'
FixNamespaceComments: 'true'
IncludeBlocks: Regroup
IndentCaseLabels: 'true'
IndentWidth: '4'
IndentWrappedFunctionNames: 'false'
KeepEmptyLinesAtTheStartOfBlocks: 'false'
Language: Cpp
MaxEmptyLinesToKeep: '2'
NamespaceIndentation: None
PointerAlignment: Right
ReflowComments: 'true'
SortIncludes: 'false'
SortUsingDeclarations: 'false'
SpaceAfterCStyleCast: 'true'
SpaceAfterLogicalNot: 'false'
SpaceAfterTemplateKeyword: 'false'
SpaceBeforeAssignmentOperators: 'true'
SpaceBeforeCpp11BracedList: 'false'
SpaceBeforeCtorInitializerColon: 'false'
SpaceBeforeRangeBasedForLoopColon: 'true'
SpaceInEmptyParentheses: 'false'
SpacesBeforeTrailingComments: '1'
SpacesInAngles: 'false'
SpacesInCStyleCastParentheses: 'false'
SpacesInContainerLiterals: 'false'
SpacesInParentheses: 'false'
SpacesInSquareBrackets: 'false'
Standard: Cpp11
TabWidth: '4'
UseTab: ForContinuationAndIndentation

It's pretty close to what we have, Some of the things are a bit annoying, like braces/alignment/wrapping around short control statements/functions. I guess we're gonna have to live with it.

[salkinium]

Oh and clang-format won't work on the template files. But they have the .in file ending, so they should automatically be excluded? Perhaps we should run clang format on a few example outputs too?

[henrikssn]

Setting BreakBeforeBraces: WebKit seems to fix the annoying brace wrapping everywhere but keep it on function declarations.

I think we have to live with it being disabled for template classes, I try to minimize using templates anyway since it causes problems with all tooling that assumes C++ syntax (i.e. also my IDE).

Is the tab width really 120? That seems quite extreme to me, even Java style only has 100 (to account for the ClassNamesThatAreWayTooLongFactoryImplProvider)

[salkinium]

I think we should test it on parts of the repository to see what config minimizes the changes. Seems easier to me to reverse engineer our actual style that way.

Is the tab width really 120?

The ColumnLimit is 120 yes, but it's more of a soft recommendation. I often found not brute-forcing the code into a rectangle shape for the sake of it to be better for understanding.
We can probably just remove it completely, and just manually check for very long lines.

[salkinium]

Is the tab width really 120?

Hold on, this is not true at all, my editor is set to wrap at 80, with 120 being just a marker line.
So I guess we can set this to 80. Good catch.

[salkinium]

But browsing through the source code, there's a lot of code that's just over 80 and it would hurt readability to force a line break there. I think we should just disable this check completely.

[salkinium]

I'm closing this since we added a .clang-format in the meantime.