Add a .clang-format file implementing the Icinga 2 style guide

[jschmidt-icinga]

This adds my relatively minimal .clang-format file to the project.

Most options correspond directly to requirements made in the style guide or conventions I've observed in the code-base or been reminded of in reviews.

BasedOnStyle: LLVM

Our style-guide directly states "we follow the clang format", which is this style as a base.

AccessModifierOffset: -4

private:, public: etc. align with the outer scope of the class.

AlignAfterOpenBracket: DontAlign

We want to always just add one level of indentation when continuing function call parameter lists etc., not align. I don't agree with this, but I can follow it.

BreakBeforeBraces: Mozilla

Adds some nice inconsistency so if conditions (etc.) don't have the braces on the next line like everything else. 👨‍🍳👌

ColumnLimit: 100

Arbitrarily chosen by me, as the style guide doesn't make a recommendation and our code is all over the place. It should be noted that clang-format doesn't have a concept of "maybe break here because it looks nicer". If we choose a larger value, it will reformat all lines to be as long as the other rules allow, if we chose a smaller value, it will force things to be broken up with increasing amounts of ugliness.

IncludeCategories:
[...]

Ordered as the style guide requires.

IndentCaseLabels: true

A convention in our code, though not covered by the style guide under the "we follow the clang format" condition.

IndentWidth: 4

Indents are 4 characters wide (on the screen, doesn't mean it has to add 4 spaces, more on that below).

PenaltyBreakAssignment: 50
PenaltyBreakBeforeFirstCallParameter: 50
PenaltyBreakOpenParenthesis: 50

Clang-format by default tends to prefer to break up long lines as early as possible if they need to be broken at all. Though there are a few examples where this is done in our code, the general convention seems to be to break the lines when above a certain threshold.

PointerAlignment: Left
ReferenceAlignment: Left

Self-explanatory. This is what got this PR started in #10516.

TabWidth: 4
UseTab: Always

Note: we need both this and the IndentWidth above to get the expected behavior of each 4 character wide unit being replaced by a tab.

[julianbrost]

That'll be an ongoing review. I've added the config locally and we'll see how it goes.

[yhabteab]

And this is the format settings I use - exported from my IDE :), is identical but not exactly the same as yours :). Most of the time it's formatted automatically but sometimes I've to still adjust some formatting choices manually.

CLion (clang-format)

---
Language: Cpp
BasedOnStyle: LLVM
AccessModifierOffset: -4
AlignAfterOpenBracket: AlwaysBreak
AlignConsecutiveAssignments: false
AlignConsecutiveDeclarations: false
AlignOperands: true
AlignTrailingComments: false
AlwaysBreakTemplateDeclarations: Yes
BraceWrapping: 
  AfterCaseLabel: false
  AfterClass: true
  AfterControlStatement: false
  AfterEnum: true
  AfterFunction: true
  AfterNamespace: true
  AfterStruct: true
  AfterUnion: true
  AfterExternBlock: false
  BeforeCatch: false
  BeforeElse: false
  BeforeLambdaBody: false
  BeforeWhile: false
  SplitEmptyFunction: false
  SplitEmptyRecord: false
  SplitEmptyNamespace: false
BreakBeforeBraces: Custom
BreakConstructorInitializers: AfterColon
BreakConstructorInitializersBeforeComma: false
ColumnLimit: 120
ConstructorInitializerAllOnOneLineOrOnePerLine: false
IncludeCategories: 
  - Regex: '^<.*' # System headers
    Priority: 1
  - Regex: '^".*' # Local headers
    Priority: 2
  - Regex: '.*' # Catch-all for any other includes
    Priority: 3
IncludeIsMainRegex: '([-_](test|unittest))?$'
IndentCaseLabels: true
IndentWidth: 4
InsertNewlineAtEOF: true
MacroBlockBegin: ''
MacroBlockEnd: ''
PointerAlignment: Left
SpaceAfterCStyleCast: true
SpaceAfterTemplateKeyword: false
SpaceInEmptyParentheses: false
SpacesInAngles: false
SpacesInConditionalStatement: false
SpacesInCStyleCastParentheses: false
SpacesInParentheses: false
TabWidth: 4
UseTab: Always
...

[jschmidt-icinga]

@yhabteab I'm using this site to validate and test the configuration. When I put in your configuration it show that there are several deprecated options it can migrate and one invalid one (BreakConstructorInitializersBeforeComma, which I can't find in the documentation either).

With regards to the padding and alignment options in your config, I haven't seen a lot of that in our code-base. Would you say that's a convention?

[yhabteab]

When I put in your configuration it show that there are several deprecated options it can migrate and one invalid one (BreakConstructorInitializersBeforeComma, which I can't find in the documentation either).

I'm not quite sure, the resulting yml file is exported from my CLion settings, so I never explicitly used such a custom clang-format file. However, I was only able to find the BreakConstructorInitializersBeforeComma option with Clang 4.0 :), so it's quite old.

Would you say that's a convention?

Nothing that I use is a convention :)! I just subjectively configured my IDE to format the code in a similar manner to the existing Icinga 2 code base.

[jschmidt-icinga]

Do we want alignment to be done with tabs or spaces?

A value of UseTab: ForIndentation will correctly align this¹:

struct HttpServerConnectionFixture : TlsStreamFixture,
                                     IcingaApplicationFixture,
                                     ConfigurationCacheDirFixture,
                                     TestLoggerFixture
{

However that also means something like this will get aligned with spaces, even though it's the same as an indentation step:

const boost::filesystem::path CertificateFixture::m_PersistentCertsDir =
    boost::filesystem::current_path() / "persistent" / "certs";

There doesn't seem to be any option to make this an actual level of indentation instead. As I said, clang-format is pretty limited sometimes. Also this is why I personally prefer spaces for everything, tabs always cause problems with consistency.

Footnotes

1. On that note, how would we want to break these in general, like this, before or after the colon (see here)? ↩

[julianbrost]

For me, the first is alignment whereas the second isn't, but rather indentation for continued lines. I'd only consider it alignment if there was a function call and the second line was aligned to match the ( of the function call.

On that note, how would we want to break these in general, like this, before or after the colon (see here)?

I mean the other three variants shown there don't need to align it, so something like that would be fine for me, and it would also be more similar to how we indent the initialization of members in constructor (which uses a similar : ..., ..., syntax):

icinga2/lib/remote/jsonrpcconnection.cpp

Lines 38 to 44 in d98caf2

	JsonRpcConnection::JsonRpcConnection(const WaitGroup::Ptr& waitGroup, const String& identity, bool authenticated,
	const Shared<AsioTlsStream>::Ptr& stream, ConnectionRole role, boost::asio::io_context& io)
	: m_Identity(identity), m_Authenticated(authenticated), m_Stream(stream), m_Role(role),
	m_Timestamp(Utility::GetTime()), m_Seen(Utility::GetTime()), m_IoStrand(io),
	m_OutgoingMessagesQueued(io), m_WriterDone(io), m_ShuttingDown(false), m_WaitGroup(waitGroup),
	m_CheckLivenessTimer(io), m_HeartbeatTimer(io)
	{

[julianbrost]

I'd go for 120 here, I think that better matches the current reality (at least there's quite a bit code that uses that width, of course there's (older) code that restricts itself to less, but in general, 120 chars seems to be a quite common limit nowadays)

[jschmidt-icinga]

I've since raised it to 112 locally, because yes, 100 is way too low. 120 is fine too, but wraps lines on my 24" monitors at home 😆.

[jschmidt-icinga]

For me, the first is alignment whereas the second isn't

Yeah... for clang-format it seems to be, or maybe a "continuation", but that seems to be the same for both these cases too.

At least with respect to the UseTabs value, because otherwise it doesn't matter much. Sadly BreakInheritanceList: BeforeColon (which is the default) doesn't seem to work as expected. I'll look into into why and then maybe we can keep UseTabs: Always and pretend that these alignments are indents.

[jschmidt-icinga]

I'll look into into why and then maybe we can keep UseTabs: Always and pretend that these alignments are indents.

I've wasted some more time on this and it definitely seems to be a bug in clang-format. BreakInheritanceList: BeforeColon seems to always behave identically to BreakInheritanceList: AfterColon, with both giving the desired outcome when ColumnLimit is reduced to a low enough value to force the line break. The other values of BreakInheritanceList work as expected. 🤷

I'll leave both settings (BreakInheritanceList to default and UseTabs: Always) in place for now and in my PR I'll just accept the output it gives me, even though its ugly with the tab alignment.

[jschmidt-icinga]

Another example where clang-format is less than satisfactory:

for (auto& configObj : DependencyGraph::GetChildren(obj)) {
	used_by->Add(new Dictionary({{"type", configObj->GetReflectionType()->GetName()},
		{"name", configObj->GetName()}}));
}