Preview of clang-format changes and CI

[Flamefire]

NOT MEANT FOR MERGING
Once this is approved the style will be applied to #88 instead as otherwise conflicts would be unresolvable

@mat007 as you wrote at #88 (comment) you don't really care what style is used as long as it is consistent. I therefore imported a clang-format style from another project and only slightly modified it.

Doesn’t Boost suggest a clangformat file? Or some of the library authors?

Not AFAIK. There are 7-8 libs with clang-format files and they don't look complete, so it's hard to tell...
When I took over maintenance of Boost.Nowide I created a style file that closely matches the style of the author. So there are a few options here:

• Take one of the other Boost lib clang-format files
• Take the one from Boost.Nowide (I like to have them created by --dump-config so everything is explicit and upgrading clang-format is easier)
• Take this one (from https://github.com/Return-To-The-Roots/s25client)
• I try to match current style
• Improve this one to match your liking

I'd go with the last option so here a few key points:

• Using clang-format-10 (major versions require different clang-format files. 10 is the most current, available in Ubuntu 18.04 and as static builds at e.g. https://github.com/muttleyxd/clang-format-static-binaries/releases)
• line length: 120
• closing namespace comments
• wrap before { except for namespace and short blocks (put on single line)
• indent of 4 spaces
• spaces around operators
• no spaces in brackets
• no indent on outermost namespace
• nested namespace on same line: namespace mock { namespace detail {, short of syntax like namespace mock::detail {

@mat007 Just check the above points and the changed files and tell me where you'd like a change. E.g. I don't like the spaces in brackets and it seems to not be used in Boost, e.g. https://github.com/boostorg/outcome/blob/master/include/boost/outcome/basic_outcome.hpp. But as you had them I can add them back. Can be adjusted separately for conditions, functions, calls, templates if wanted

[mat007]

The original formatting of the code was heavily influenced by the practices at the company I was working for at the time.
I don’t mind changing it entirely at all.

I think we should pick a format that requires as little configuration override as possible.
It looks like the one you use in Boost.Nowide fits that requirement?

[Flamefire]

I think we should pick a format that requires as little configuration override as possible.

What exactly do you mean by that? As mentioned the config is generated by --dump-config so it contains all settings. This is very valuable when upgrading clang-format versions. So the file doesn't get smaller when overriding less. In fact I don't even know what is "original" and what is not. You got a couple pre-defined styles and my config is based on one of them but don't remember which one. I tried not to change much

It looks like the one you use in Boost.Nowide fits that requirement?

That one was generated the same: Start from some config then start changing settings so the source code formatting stays as similar as possible. You can however compare the resulting style and choose one or the other.

[mat007]

What exactly do you mean by that?

I guess I meant let’s dump the config and that’s it, try not to override anything if that’s possible?

[Flamefire]

Then the question is: What is "the config"? clang-format has predefined styles: LLVM, Google, Chromium, Mozilla, WebKit, each is an own set of settings. The config here is based on one of them (can't remember which) but adjusted for 2 things: minimizing horizontal and vertical spacing (e.g. the removal of spaces in parentheses or putting short functions [mostly getters] onto single lines) while maximizing readability, especially related to control flow: braces on new line, block indented, however also use } else as I consider that "1 token"

I don't see a reason "to not override anything" from the default styles. The file has the same size. Everyone has a different taste and I guess each of those configs is simply adjusted to be similar to whatever they used when they introduced clang-format. I see most styles use a TabWidth of 8 which I find wasteful. Some don't have a column limit or use only 80 which IMO is to small for modern systems. 120 is a good compromise

I added a commit with the styles without changes (no file changes though) and the diffs to the current style is: format.txt

Diff to e.g. the nowide format:

11,14c11,14
< AllowAllArgumentsOnNextLine: true
< AllowAllConstructorInitializersOnNextLine: true
< AllowAllParametersOfDeclarationOnNextLine: true
< AllowShortBlocksOnASingleLine: Never
---
> AllowAllArgumentsOnNextLine: false
> AllowAllConstructorInitializersOnNextLine: false
> AllowAllParametersOfDeclarationOnNextLine: false
> AllowShortBlocksOnASingleLine: false
16c16
< AllowShortFunctionsOnASingleLine: Inline
---
> AllowShortFunctionsOnASingleLine: None
24,25c24,25
< BinPackArguments: true
< BinPackParameters: true
---
> BinPackArguments: false
> BinPackParameters: false
27c27
<   AfterCaseLabel:  true
---
>   AfterCaseLabel:  false
36c36
<   AfterExternBlock: true
---
>   AfterExternBlock: false
46c46
< BreakInheritanceList: AfterColon
---
> BreakInheritanceList: BeforeColon
49c49
< BreakConstructorInitializers: BeforeColon
---
> BreakConstructorInitializers: AfterColon
54c54
< CompactNamespaces: true
---
> CompactNamespaces: false
59d58
< DeriveLineEnding: true
81,86c79,81
<     SortPriority:    0
< IncludeIsMainRegex: '(Impl|_Win32|_Other)?$'
< IncludeIsMainSourceRegex: ''
< IndentCaseLabels: true
< IndentGotoLabels: true
< IndentPPDirectives: AfterHash
---
> IncludeIsMainRegex: '(test)?$'
> IndentCaseLabels: false
> IndentPPDirectives: None
111c106
< SortUsingDeclarations: true
---
> SortUsingDeclarations: false
121d115
< SpaceInEmptyBlock: false
125d118
< SpacesInConditionalStatement: false
130,134c123,124
< SpaceBeforeSquareBrackets: false
< Standard:        c++14
< StatementMacros:
<   - Q_UNUSED
<   - QT_REQUIRE_VERSION
---
> Standard:        Cpp03
> StatementMacros: []
136d125
< UseCRLF:         false
139d127
< 

I mean that's the whole point of clang-format: You can adapt to the way you think is best. Just check the code for something you don't like and change it in the format file. Or for this you can check the diffs and decide which change you'd like to revert.
But IMO in the end it does not matter. Except for line length of 100-120, tab width of 4 and braces on new lines I don't mind any style.

Hope that helps

[mat007]

But IMO in the end it does not matter. Except for line length of 100-120, tab width of 4 and braces on new lines I don't mind any style.

Yes it doesn’t matter, as long as it can be checked by the CI and applied with one command on the whole source tree.
I’m also fine with whatever style you may choose as long as it doesn’t involve a lot of config changes.
Just picking one of the predefined style sounds best to me, but if you want to tweak a couple of things on top that’s OK too.

[Flamefire]

I’m also fine with whatever style you may choose as long as it doesn’t involve a lot of config changes.

I'm wondering what the reasoning behind this is. Is it to make it match an existing style so people are more likely to be used to it?
Or is it so the config file stays small? In that case shall the file only contain the changed things? That makes it (a bit) harder to upgrade, hence I like including the full --dump-config output. But then the "lot of changes" isn't even visible.
Thanks for the answer, just want to make sure I'm on the right track and there is no misunderstanding

[mat007]

I'm wondering what the reasoning behind this is. Is it to make it match an existing style so people are more likely to be used to it?

It’s more I don’t think it’s worth spending time tweaking a config. I assume the predefined configs all make a good job at providing a set of meaningful coherent rules and I’m sure it’s easy to mentally adjust to any of them.

[Flamefire]

Ok, I updated it based on the Mozilla style (closest to the one I had first) with the following changes and reasoning:

• ColumnLimit: 120 instead of 80 (modern monitors are wide)
• IndentWidth: 4 instead of 2 (wider column limit allows to make indent more visible)
  • AccessModifierOffset: -4 instead of -2
  • ConstructorInitializerIndentWidth: 4 instead of 2
• AlignEscapedNewlines: Left instead of Right (IMO putting them right makes you miss them)
• AlwaysBreakAfterReturnType: None instead of TopLevel (Don't like break after return type and only using TopLevel feels inconsistent. How would you explain that?)
• BraceWrapping
  • AfterCaseLabel, AfterControlStatement, AfterObjCDeclaration: true (for consistency: braces go on newline)
  • SplitEmptyFunction, SplitEmptyNamespace: false: (Why waste a line?)
• BreakInheritanceList: AfterColon instead of BeforeComma (I find the comma+space at the start of the line disturbing)
• BreakConstructorInitializers: BeforeColon instead of BeforeComma (same)
• CompactNamespaces: true (Recent addition, Mozilla style was likely not updated and I find that really useful)
• FixNamespaceComments: true (useful or they become outdated/incomplete)
• IncludeCategories/StatementMacros: Adjusted for turtle, this is very project specific
• IndentPPDirectives: AfterHash instead of None (Recent addition. IMO useful)
• KeepEmptyLinesAtTheStartOfBlocks: false (Why keep them? Likely an omission)
• NamespaceIndentation: Inner instead of None (Namespaces are blocks like classes. So this makes it possible to distinguish levels of namespaces (e.g. detail namespace). I guess Mozilla has 1 namespace per file, then it doesn't matter)
• SpaceBeforeParens: Never instead of ControlStatements (For consistency, easier to just say "never")
• Standard: c++14 (Clang-Format 10 addition)

See https://releases.llvm.org/10.0.0/tools/clang/docs/ClangFormatStyleOptions.html for details on the options.

As you can see most changes are either adjustment for wider columns, new additions to clang-format or for increased consistency of overall rules (like: Brace goes on new line)

What do you say?

[mat007]

Works for me, thanks!

[Flamefire]

Great! Closed this then and will rebase and apply when #88 is ready to be merged or better after it is merged due to the amount of changes coming in. Final suggestion here: Especially for the long parameter list tests the formatting is suboptimal as with the BinPack*: false option it places each on a new line. This might be good for a couple parameters but for those with >30 it is awful. We could either enable that option or disable formatting for those long parameter tests. Or just ignore it, but I wanted to bring it up.

Let's continue the rest in #88, e.g. I need to know when to apply this formatting.