I'm not sure how this works, but would it be possible to have a target that only works on a patch? You usually want to make sure what you modified didn't diverge from the code.

There is probably a way to run clang-format only on a patch. See http://clang.llvm.org/docs/ClangFormat.html#script-for-patch-reformatting

But should we not prefer running clang format one time, instead of having only some part of the code auto-formatted?

I'm all for it. This would unify how we can reformat any KDE module, which is very much desirable.

Being able to just reformat a patch sounds interesting, too, but can be added later still.

Don't let the perfect be the enemy of the good...

The example usage should go to the location that Carl pointed out.

+1, can we have another iteration?

In D24568#545736, @apol wrote:

I'm not sure how this works, but would it be possible to have a target that only works on a patch? You usually want to make sure what you modified didn't diverge from the code.

I think there is some hack around that:
http://clang.llvm.org/docs/ClangFormat.html#script-for-patch-reformatting

But actually, if your sources are already clang-formatted, you just need to run the clang-format target once before you commit, the your new code will be the only thing altered.

If there are more deviations from the kdelibs/frameworks coding style, please tell me.

Otherwise, I am happy with the output of this file for KTextEditor & Kate.

For the .clang-format instantiation question: I think it makes sense to instantiate it on initial cmake running, as otherwise the tooling users use for clang-format/clangd will not pick up the style.

common KDE style file

There's no such thing as a common KDE style

In D24568#546227, @aacid wrote:

common KDE style file

There's no such thing as a common KDE style

Shall we name it kdelibs coding style?
But actually the idea is to have one, as opt in, for the repos that want that.

If the KDE Frameworks developers have agreed to a common style, then yes, naming it KDE Frameworks style makes sense :)

I hope this file implements https://community.kde.org/Policies/Kdelibs_Coding_Style

And that is the style all frameworks got astyled with on initial import.

There is also https://techbase.kde.org/Policies/Frameworks_Coding_Style which though missed the move from techbase to community, other than the other policies.

I suspect that page should be moved over now as well, to become the real KF coding style page (so that "kdelibs" named one is no longer needed).

@nalvarez @ochurlaud Do you remember why this page was not moved?

In D24568#546289, @kossebau wrote:

There is also https://techbase.kde.org/Policies/Frameworks_Coding_Style which though missed the move from techbase to community, other than the other policies.

I suspect that page should be moved over now as well, to become the real KF coding style page (so that "kdelibs" named one is no longer needed).

@nalvarez @ochurlaud Do you remember why this page was not moved?

This must have been a mistake. Please make sure it's findable on community.ko!

In D24568#545942, @cullmann wrote:

In D24568#545736, @apol wrote:

I'm not sure how this works, but would it be possible to have a target that only works on a patch? You usually want to make sure what you modified didn't diverge from the code.

I think there is some hack around that:
http://clang.llvm.org/docs/ClangFormat.html#script-for-patch-reformatting

But actually, if your sources are already clang-formatted, you just need to run the clang-format target once before you commit, the your new code will be the only thing altered.

For our projects at work where we have the liberty to dictate the coding style, we also use clang format. To do that properly, we have a pre-commit check locally and then a gerrit bot similar to the Qt coding style bot which checks the formatting introduced by a patch. This way, one can be sure that the style doesn't deteriorate over time.

Some links on that matter:

• https://github.com/nghttp2/nghttp2/blob/master/pre-commit is what I use to check my commits, there are probably other, better hooks available somewhere, but this one has suited me well so far
• when the hook complains, I run git clang-format which is part of the clang package that also ships clang-format on ArchLinux

I guess the commit hook can also be used for a server check, but I'm not sure how this is done with gerrit for us, or how one would do it with phabricator/gitlab for KDE.

Anyhow: big +1 form my side for using clang-format for all of KDE (eventually) and KDE frameworks in the near future as a starting point.

Perhaps David could give feedback if the file actually captures the intend to do proper KDE Frameworks/libs like formatting.
I had a mistake with the indented case statements, that should be fixed.

Do we want these, found in https://code.qt.io/cgit/qt/qt5.git/tree/_clang-format?

# We use template< without space.
SpaceAfterTemplateKeyword: false

# macros for which the opening brace stays attached.
ForEachMacros:   [ foreach, Q_FOREACH, BOOST_FOREACH, forever, Q_FOREVER, QBENCHMARK, QBENCHMARK_ONCE ]

# Break constructor initializers before the colon and after the commas.
BreakConstructorInitializers: BeforeColon

In general, I'm curious why we're not using qt5's clang-format file, with our only difference (braces for single-line statements) on top?

In D24568#548490, @dfaure wrote:

Do we want these, found in https://code.qt.io/cgit/qt/qt5.git/tree/_clang-format?

# We use template< without space.
SpaceAfterTemplateKeyword: false

# macros for which the opening brace stays attached.
ForEachMacros:   [ foreach, Q_FOREACH, BOOST_FOREACH, forever, Q_FOREVER, QBENCHMARK, QBENCHMARK_ONCE ]

# Break constructor initializers before the colon and after the commas.
BreakConstructorInitializers: BeforeColon

added that

In general, I'm curious why we're not using qt5's clang-format file, with our only difference (braces for single-line statements) on top?

I was more comfortable tweaking the file I created some years ago (and updated for all clang-format versions since then) that should be close to the kdelibs style than trying to take a look in detail at the other one, to be honest ;=)

With

BreakConstructorInitializers: BeforeColon

you get collapsed stuff like;

Range::Range(const KTextEditor::Cursor &c1, const KTextEditor::Cursor c2, MotionType mt) : Range(c1.line(), c1.column(), c2.line(), c2.column(), mt)

I think the behavior of the default of WebKit

BCIS_BeforeComma (in configuration: BeforeComma) Break constructor initializers before the colon and commas, and align the commas with the colon.

Constructor()

: initializer1()
, initializer2()

is much more reasonable.

One can play with

ConstructorInitializerAllOnOneLineOrOnePerLine (bool)
If the constructor initializers don’t fit on a line, put each initializer on its own line.

true:
SomeClass::Constructor()

  : aaaaaaaa(aaaaaaaa), aaaaaaaa(aaaaaaaa), aaaaaaaa(aaaaaaaaaaaaaaaaaaaaaaaaa) {
return 0;

}

false:
SomeClass::Constructor()

  : aaaaaaaa(aaaaaaaa), aaaaaaaa(aaaaaaaa),
    aaaaaaaa(aaaaaaaaaaaaaaaaaaaaaaaaa) {
return 0;

}
ConstructorInitializerIndentWidth (unsigned)
The number of characters to use for indentation of constructor initializer lists as well as inheritance lists.

Without the initializer change, the file works for me reasonable well, tried it again on KTextEditor.

Has somebody tested the current state?

I really would like to get this going as in my eyes this will finally put a working solution in place for the coding style enforcing on at least frameworks.

One can later still improve stuff like "shall the gitlab stuff execute that on the patches" or "shall we let scripty run this once a month over all repositories that opt-in to this" to keep the style consistent.

At company we just have the rule that one is free to run the company wide reformat target and commit that as some extra pure "reformat" commit if the coding style did deteriorate.

Ok, I pushed this now.

I will use that myself in KTextEditor and Kate instead of my old file and shell script.

We can think about what to do with other frameworks now.

Volker Ok'd it that I used that on the co-maintained KSyntaxHighlighting framework, too.

As an update on this from the Plasma POV.

I added the macro to every repo and told every dev to do a final test before we commit the formatted results.

I had some feedback and the result was that we can't proceed with in the current state [1].

What's noteworthy is we were generally ok with the results from the first file we prepared in T11214, so potentially we just need some settings tweaked.
I'll try and break that down into future diffs.

1.https://mail.kde.org/pipermail/plasma-devel/2019-November/106186.html

You can force the current clang format to keep the multi-line if as follows:

if (roles.isEmpty() // comment
        || roles.contains(Notifications::UpdatedRole)
        || roles.contains(Notifications::ExpiredRole)

The comment prevents clang to join the lines. I understand it's not what you want, though.

In D24568#563092, @davidedmundson wrote:

As an update on this from the Plasma POV.

I added the macro to every repo and told every dev to do a final test before we commit the formatted results.

I had some feedback and the result was that we can't proceed with in the current state [1].

What's noteworthy is we were generally ok with the results from the first file we prepared in T11214, so potentially we just need some settings tweaked.
I'll try and break that down into future diffs.

1.https://mail.kde.org/pipermail/plasma-devel/2019-November/106186.html

For the comment alignment stuff, that is easy, we can just remove

AlignTrailingComments: true

For the other general "moving between different lines" stuff, I don't see any solution for this.

You can arbitrary change the ColumnLimit, but as far as I know, it will just arbitrarily pack stuff together, like in the if conditions, if you have not e.g. comments in-between.

But perhaps somebody else knows a proper switch for that.

For the lambda issue, I think we can add:

# keep lambda formatting multi-line if not empty
AllowShortLambdasOnASingleLine: Empty

see https://clang.llvm.org/docs/ClangFormatStyleOptions.html

That fixes for me the issue with the collapsed lambda.

For the comments, as said, we can remove

AlignTrailingComments: true

For the generic issue of stuff folded in one line, one can play with the ColumnLimit, but as said, that will still arbitrarily pack stuff as long as it fits the limit.

Kai, could you try the two changes (for lambda and comments) by just temporarily changing that in the .clang-format file locally and rerunning make clang-format?

Btw., I just tried e.g.

https://code.qt.io/cgit/qt/qt5.git/tree/_clang-format

that one has exactly the same issues as our file and re-flows stuff into single lines (even lambdas).

The only thing that often hinders that is the column limit of 100 there.

That on the other side make as lot of code lot harder to read, too, like:

-    cc()->unregisterCompletionModel(KTextEditor::EditorPrivate::self()->wordCompletionModel()); // would add additional items, we don't want that in tests
+    cc()->unregisterCompletionModel(KTextEditor::EditorPrivate::self()
+                                            ->wordCompletionModel()); // would add additional items,
+                                                                      // we don't want that in tests

-    highlightMatchAttribute->dynamicAttribute(Attribute::ActivateMouseIn)->setBackground(searchColor);
-    highlightMatchAttribute->dynamicAttribute(Attribute::ActivateMouseIn)->setForeground(foregroundColor);
-    highlightMatchAttribute->dynamicAttribute(Attribute::ActivateCaretIn)->setBackground(searchColor);
-    highlightMatchAttribute->dynamicAttribute(Attribute::ActivateCaretIn)->setForeground(foregroundColor);
+    highlightMatchAttribute->dynamicAttribute(Attribute::ActivateMouseIn)
+            ->setBackground(searchColor);
+    highlightMatchAttribute->dynamicAttribute(Attribute::ActivateMouseIn)
+            ->setForeground(foregroundColor);
+    highlightMatchAttribute->dynamicAttribute(Attribute::ActivateCaretIn)
+            ->setBackground(searchColor);
+    highlightMatchAttribute->dynamicAttribute(Attribute::ActivateCaretIn)
+            ->setForeground(foregroundColor);

(and that is just one of .... places in KTextEditor that look like that afterwards).

This has been missing the link from an rst file in docs/, so the documentation generation picks up the file. Fixed with c4890d5c03ed79f0c87da861b6608bbd46c2162c

In D24568#580338, @kossebau wrote:

This has been missing the link from an rst file in docs/, so the documentation generation picks up the file. Fixed with c4890d5c03ed79f0c87da861b6608bbd46c2162c

Thanks for adding that!

Any feedback on the other issues people?

Ping? Any update if somebody tried the proposed changes?

In D24568#563653, @cullmann wrote:

For the lambda issue, I think we can add:

# keep lambda formatting multi-line if not empty
AllowShortLambdasOnASingleLine: Empty

see https://clang.llvm.org/docs/ClangFormatStyleOptions.html

That fixes for me the issue with the collapsed lambda.

For the comments, as said, we can remove

AlignTrailingComments: true

For the generic issue of stuff folded in one line, one can play with the ColumnLimit, but as said, that will still arbitrarily pack stuff as long as it fits the limit.

Kai, could you try the two changes (for lambda and comments) by just temporarily changing that in the .clang-format file locally and rerunning make clang-format?

Hi, would it be ok to add this two improvements?

Beside that, has somebody tried the formatting again?