Provide clang-format target with a KDE Frameworks style file
ClosedPublic

Authored by cullmann on Oct 11 2019, 6:46 PM.

Details

Summary

Provides a clang-format target if wanted

Example usage:

include(KDEClangFormat)

add clang-format target for all our real source files

file(GLOB_RECURSE ALL_CLANG_FORMAT_SOURCE_FILES autotests/src/*.cpp autotests/src/*.h src/*.cpp src/*.h templates/*.cpp templates/*.h)
kde_clang_format(${ALL_CLANG_FORMAT_SOURCE_FILES})

Test Plan

Tried that above usage thingy in KTextEditor

Diff Detail

Repository
R240 Extra CMake Modules
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
cullmann created this revision.Oct 11 2019, 6:46 PM
Restricted Application added projects: Frameworks, Build System. · View Herald TranscriptOct 11 2019, 6:46 PM
Restricted Application added subscribers: kde-buildsystem, kde-frameworks-devel. · View Herald Transcript
cullmann requested review of this revision.Oct 11 2019, 6:46 PM
ognarb added a subscriber: ognarb.Oct 11 2019, 9:24 PM
ognarb added inline comments.
kde-modules/KDEClangFormat.cmake
12

need doc

apol added a subscriber: apol.Oct 11 2019, 11:37 PM

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.

davidedmundson added inline comments.
kde-modules/KDEClangFormat.cmake
49

it's a bit weird to put new files in the source directory when running cmake.

Copying when running the target clang-format would probably be ok as there you're already doing an explicit operation which changes the source.

cullmann added inline comments.Oct 12 2019, 11:17 AM
kde-modules/KDEClangFormat.cmake
49

If we don't do this per default, user clang-format tooling like via LSP in Atom/Vscode/Kate will not work properly before a manual action.

cullmann updated this revision to Diff 67796.Oct 12 2019, 4:03 PM
  • fix coding style issue, we don't want indented case labels
cullmann updated this revision to Diff 67797.Oct 12 2019, 4:07 PM
  • add initial docs
cullmann marked an inline comment as done.Oct 12 2019, 4:08 PM
cullmann added inline comments.
kde-modules/KDEClangFormat.cmake
12

Added some initial docs

cullmann marked an inline comment as done.

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.

aacid added a subscriber: aacid.Oct 12 2019, 9:52 PM

common KDE style file

There's no such thing as a common KDE style

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.

kossebau added a subscriber: kossebau.EditedOct 12 2019, 11:31 PM

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?

cullmann retitled this revision from Provide clang-format target with a common KDE style file to Provide clang-format target with a KDE Frameworks style file.Oct 13 2019, 11:34 AM

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!

mwolff added a subscriber: mwolff.Sun, Oct 13, 7:24 PM
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?

kde-modules/clang-format.cmake
43

Typo: semantics

44

I'm confused because here it says "true" will group via empty lines, while https://code.qt.io/cgit/qt/qt5.git/tree/_clang-format says the SortInclude feature of clang-format does not re-order includes separated by empty lines. Maybe different versions of clang-format? Or I misunderstand something?

50

https://code.qt.io/cgit/qt/qt5.git/tree/_clang-format says PointerBindsToType: false, what's the difference?

76

typo: ternary

dfaure requested changes to this revision.Wed, Oct 16, 8:42 PM

I like the idea very much.

kde-modules/KDEClangFormat.cmake
77

I wonder if people compiling KF5 modules (and not necessarily planning on contributing) need to be annoyed with such a warning. Maybe we could still define the clang-format target and make it print an error?

This revision now requires changes to proceed.Wed, Oct 16, 8:42 PM
cullmann updated this revision to Diff 68091.Wed, Oct 16, 9:20 PM
cullmann marked 4 inline comments as done.
  • fix coding style issue, we don't want indented case labels
  • add initial docs
  • adjust style
  • just tell the user it will not work

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 ;=)

kde-modules/KDEClangFormat.cmake
77

Yeah, I think some message on running the target will be less annoying.

kde-modules/clang-format.cmake
44

Perhaps my comment is not understandable.
true means clang-format will sort stuff inside #include groups (aka #include lines without empty lines in-between)
Will alter the comment.

50

My file is for some recent clang, whereas PointerBindsToType seems to be ancient, see:

https://releases.llvm.org/3.4/tools/clang/docs/ClangFormatStyleOptions.html (there you still have that)

http://releases.llvm.org/9.0.0/tools/clang/docs/ClangFormatStyleOptions.html (here I can only find the variant I use)

sitter added a subscriber: sitter.Thu, Oct 17, 9:33 AM
sitter added inline comments.
kde-modules/KDEClangFormat.cmake
54

I'm pretty sure you need to check the version the exectuable. When I use 6.0 I get ctors smushed into one line.

cullmann marked an inline comment as done.Thu, Oct 17, 6:28 PM

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.

kde-modules/KDEClangFormat.cmake
54

No, actually the reason for that is the added BreakConstructorInitializers: BeforeColon
I am not sure how to avoid that if we not go back to the old variant I had without that.

cullmann updated this revision to Diff 68186.Thu, Oct 17, 6:31 PM
cullmann marked an inline comment as done.
  • avoid collapsing of constructor initializer lines

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

zzag added a subscriber: zzag.Thu, Oct 17, 6:56 PM
zzag added inline comments.
kde-modules/clang-format.cmake
76

I've been always wondering how one should break long ternary operators when writing KF code. There are several ways to do it

(a) w/o breaking (BreakBeforeTernaryOperators: false)

const FooBar *foobar = someStupidCondition() ?
    someSuperDuperBeatifulFunctionWithLongName() :
    anotherSuperDuperBeatifuuuulFunctionWithLongName();

(b) w/ breaking (BreakBeforeTernaryOperators: true)

const FooBar *foobar = someStupidCondition()
    ? someSuperDuperBeatifulFunctionWithLongName()
    : anotherSuperDuperBeatifuuuulFunctionWithLongName();

According to the _clang-format file from the qt5 super repo, Qt fellas prefer (b) to break before ternary operators.

Do we really want to not break before ternary opeartors?

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.

kde-modules/clang-format.cmake
76

I have no strong opinion on that, we can remove that or keep it.

dfaure added inline comments.Sun, Oct 20, 10:32 AM
kde-modules/clang-format.cmake
76

(b) looks good to me, and more importantly, I like the goal of being as close as possible to the Qt coding style.

cullmann updated this revision to Diff 68369.Sun, Oct 20, 3:00 PM
  • keep default of BreakBeforeTernaryOperators = true
cullmann added inline comments.Sun, Oct 20, 3:01 PM
kde-modules/clang-format.cmake
76

removed that setting, back to Webkit default of BreakBeforeTernaryOperators: true

dfaure accepted this revision.Sun, Oct 20, 5:57 PM
This revision is now accepted and ready to land.Sun, Oct 20, 5:57 PM
This revision was automatically updated to reflect the committed changes.
cullmann marked an inline comment as done.

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.