Will formatter ruin beautiful multi-line formatting into single line?

like here

In T11214#191564, @alexeymin wrote:

Will formatter ruin beautiful multi-line formatting into single line?

like here

or

?

This will depends on the Style used. But clang format has a lot of configuration options: https://clang.llvm.org/docs/ClangFormatStyleOptions.html.

[4] https://code.qt.io/cgit/qt/qt5.git/tree/_clang-format
[5] https://github.com/qt-creator/qt-creator/tree/master/dist/clangformat#coding-rules-violated-by-clang-format

I tried my best to apply and enforce clang-format for my project, but I failed to do so in a reasonable time.
I've got an impression that clang-format configuration doesn't provide enough options to format code well enough, but maybe I should spend more hours :-).

In T11214#191565, @ognarb wrote:

In T11214#191564, @alexeymin wrote:

Will formatter ruin beautiful multi-line formatting into single line?

like here

or

?

This will depends on the Style used. But clang format has a lot of configuration options: https://clang.llvm.org/docs/ClangFormatStyleOptions.html.

Cool!
I want one member per line (I think such style is popular in KDE), what can I do? There are only true/false.

I think it would be nice to have ConstructorInitializerStyle with values 'one per line', 'all on one line', 'all on one line if fit', 'ignore' (keep as-is) and so on.

But we have only ConstructorInitializerAllOnOneLineOrOnePerLine.
It seems that MongoDB accepted that: https://github.com/mongodb/mongo/blob/master/etc/format_sample.cpp#L31

What a "well done" configurability...

Will formatter ruin beautiful multi-line formatting into single line?

In my config it'll still be multi line. Indentation of the lines can change.

But the best thing is to download the configuration above and try it.

Feel free to suggest any edits.

In T11214#191564, @alexeymin wrote:

Will formatter ruin beautiful multi-line formatting into single line?

It depends on whether ColumnLimit is set (as well as other relevant style options that control line breaking), but in general the best what you can do is just test the proposed .clang-format file for yourself and check whether multi-line formatting is ruined. ;-)

In my config it'll still be multi line.

In fact, the suggested clang-format file does the same as mine.

Probably in your sample, the last enum value ends with a comma and that force clang-format to wrap it.

I can't find such a trick for the constructor initialisation list. With both configs (your and mine) clang-format is moving members around based on the line length.
I already played with ColumnLimit and all other options from the docs and various sources. IMO the result is disappointing and clang-format is not capable to do things right :-(

I'll be happy to use the tool when it gets more reasonable options. Current keys like ConstructorInitializerAllOnOneLineOrOnePerLine: true/false make no sense.

In T11214#191682, @akulichalexandr wrote:

I already played with ColumnLimit and all other options from the docs and various sources. IMO the result is disappointing and clang-format is not capable to do things right :-(

Have you tried disabling ColumnLimit (by setting it to 0)?

In T11214#191683, @zzag wrote:

In T11214#191682, @akulichalexandr wrote:

I already played with ColumnLimit and all other options from the docs and various sources. IMO the result is disappointing and clang-format is not capable to do things right :-(

Have you tried disabling ColumnLimit (by setting it to 0)?

Yes

If I remove

BreakConstructorInitializers: BeforeColon

then I get initialisation unchanged, remaining at one per line:

+PowermanagementEngine::PowermanagementEngine(QObject *parent, const QVariantList &args)
+ : Plasma::DataEngine(parent, args)
+ , m_sources(basicSourceNames())

correctly

BreakConstructorInitializers: BeforeComma changes the output to an acceptable form.

Thanks, David. I don't have strong objections now.

At kphotoalbum, we just enabled clang-format for the code-base. So far, my experience was mostly boring (the good kind of boring).

Some pitfalls that I encountered so far:

• IncludeBlocks: Regroup, can create false positives in commit-hooks -> https://bugs.llvm.org/show_bug.cgi?id=39327
• Don't run clang-format (in-place) on symlinks (I hope this doesn't apply for most projects, but YMMV)

We want to do the same for Kate ;=)
See https://invent.kde.org/kde/kate/merge_requests/7

One other thing that we ran into in KPhotoAlbum is that apparently clang-format is not entirely stable in its results. I got slightly different results between 6.0.0, 7.0.1, and 7.1.0.
So when you create your .clang-format file you should do some testing to get consistent results across all developers.

Out of experience at work, were we "auto-reformat" stuff that way I can tell you that you can't avoid that. There are always some changes in different clang-format versions.
But normally that only affects << 1% of your sources.

For the proposed clang format.

In kate.git I now imported the own from Qt.
The only thing that I am at the moment not sure of is the column limit.
It is very short if you have long variable + function names.

We reviewed the results of the .clang-format on kate.git, we were not that happy with the re-flowing.
We tried a new variant that looks more promising, look at https://invent.kde.org/kde/kate/blob/master/.clang-format

kate.git now uses the linked file, changes were like:
https://invent.kde.org/kde/kate/commit/15a0d56b18c244fc393186f42320d6b5ac77ab94

Hmm, changes like these seem like a reduction in readability to me:

That is a un-avoidable trade off between reflow stuff and line length limit.

Same happens for large if clauses, btw. On the other side this is at least consistent.

The question is whether you really need the ColumnLimit - if you set a ColumnLimit of 0, clang-format won't reflow continuations like in Nate's first screenshot.
For Nate's second suggestion, BreakConstructorInitializers and BreakInheritanceList should produce more readable formatting.
I'm not sure which option can be used to fix the Enum formatting, but with ColumnLimit set to 0 at least the existing enums shouldn't be butchered, I guess...

For KPhotoAlbum, I've also set CommentPragmas to '^ (FALLTHROUGH|krazy:) ' to prevent comments with semantic meaning from being moved around.

From a robustness viewpoint it seems to me that SortIncludes should be enabled. Apart from uncovering bugs in header files it also helps with keeping the include list sorted. By also setting IncludeBlocks: Preserve you can still keep semantically grouped include blocks.

I just tried out the variant without column limit as attached to the other request on ktexteditor.git.
As thought you will get changes like that (with a >> 500 columns line as result)

• return toCheckState(
• currentStyle->foreground() == defaultStyle->foreground()
• && currentStyle->background() == defaultStyle->background()
• && currentStyle->selectedForeground() == defaultStyle->selectedForeground()
• && currentStyle->selectedBackground() == defaultStyle->selectedBackground()
• && currentStyle->fontBold() == defaultStyle->fontBold()
• && currentStyle->fontItalic() == defaultStyle->fontItalic()
• && currentStyle->fontUnderline() == defaultStyle->fontUnderline()
• && currentStyle->fontStrikeOut() == defaultStyle->fontStrikeOut());

+ return toCheckState(
+ currentStyle->foreground() == defaultStyle->foreground() && currentStyle->background() == defaultStyle->background() && currentStyle->selectedForeground() == defaultStyle->selectedForeground() && currentStyle->selectedBackground() == defaultStyle->selectedBackground() && currentStyle->fontBold() == defaultStyle->fontBold() && currentStyle->fontItalic() == defaultStyle->fontItalic() && currentStyle->fontUnderline() == defaultStyle->fontUnderline() && currentStyle->fontStrikeOut() == defaultStyle->fontStrikeOut());

I think you should reconsider what is worse: sometimes a bit strange line breaking or such things ;=)
In AbsInt we tried it without, too, and did run in a lot of such cases.

^ The diff for people not fluent in reverse-markdown ;)

-            return toCheckState(
-                       currentStyle->foreground() == defaultStyle->foreground()
-                       && currentStyle->background() == defaultStyle->background()
-                       && currentStyle->selectedForeground() == defaultStyle->selectedForeground()
-                       && currentStyle->selectedBackground() == defaultStyle->selectedBackground()
-                       && currentStyle->fontBold() == defaultStyle->fontBold()
-                       && currentStyle->fontItalic() == defaultStyle->fontItalic()
-                       && currentStyle->fontUnderline() == defaultStyle->fontUnderline()
-                       && currentStyle->fontStrikeOut() == defaultStyle->fontStrikeOut());
+                return toCheckState(
+                    currentStyle->foreground() == defaultStyle->foreground() && currentStyle->background() == defaultStyle->background() && currentStyle->selectedForeground() == defaultStyle->selectedForeground() && currentStyle->selectedBackground() == defaultStyle->selectedBackground() && currentStyle->fontBold() == defaultStyle->fontBold() && currentStyle->fontItalic() == defaultStyle->fontItalic() && currentStyle->fontUnderline() == defaultStyle->fontUnderline() && currentStyle->fontStrikeOut() == defaultStyle->fontStrikeOut());

Yeah, thanks ;=)

ktexteditor.git and kate.git use now btw. the file with the column limit.

If you find some consensus, we can sync again.

The file from Kate/KTextEditor seems to be spreading:

Git commit 8053b08d1806ad38a6631023f710f3ab8994af6e by Andreas Cord-Landwehr.
Committed on 06/10/2019 at 13:17.
Pushed by cordlandwehr into branch 'master'.

Use Kate's clang-format configuration

Only exception: sort include files, as one can use an empty line
to enforce separation of includes.

A +88 -0 .clang-format
A +6 -0 reformat.sh

https://commits.kde.org/artikulate/8053b08d1806ad38a6631023f710f3ab8994af6e

I synced the "sort include" change back to Kate/KTextEditor, that makes sense.

Rather than eventually duplicating this formatting definition file across dozens of repos, could we put a single copy if it in one place and have everything use it from there?

:=) If there is some consensus, this can be tried.
One issue with that is that the tooling wants to have it in the source tree.
We would e.g. add some ECM magic for that, that copies a file shipped with ECM to the source tree if we agree that the style is ok.

We could also enable the linter in Phabricator, similar to how this was done in the llvm phabricator instance: https://reviews.llvm.org/D49116

I would not spend time on Phabricator as we anyways will leave it behind not that far away ;=)

See D24568 for the draft of an ECM module that applies the Kate/KTextEditor/Artikulate style.
Is the same file as here, with sorting of includes, but with column limit.

10 days have passed. What's the status of this?

Objections were raised.

Plasma is now on pause :(

The original files here seemed to format things ok but the ones merged into frameworks caused really long lines. Mostly it's all about the column length thing above.

As noted in

https://phabricator.kde.org/D24568

1. for the lambda stuff and the comments moving, there is a simple fix, we can merge, if wanted
2. for the column wrapping, you need to try different widths and if that helps we can try to find some consensus, but only if somebody does that try and error a bit more, I pasted some examples in the other ticket.

I've just found this for QML files

https://github.com/Orange-OpenSource/qmljsreformatter

Will give it a try and report back

Maybe we can use the upstream formatter? It is available since Qt 5.15.

https://code.qt.io/cgit/qt/qtdeclarative.git/tree/tools/qmlformat

Even better.

In T11214#199606, @ngraham wrote:

Hmm, changes like these seem like a reduction in readability to me:

Can be fixed by adding // at the and, IMHO that is an even better tradeoff

Solved by adding a comma at the end.

One could manually didable clang-format for that bit, or use AlignConsecutiveAssignments (https://clang.llvm.org/docs/ClangFormatStyleOptions.html). But in a lot of places we don't use this, so I would argue that it does not make sense to enable it.

Considering that there are several improvements to the clang-format config we can proceed. We briefly discussed this on the Plasma meeting. There it was discussed that Gitlab-CI is still in alpha version and can not be used for a ton of projects.
And alternative would be to use a commit hook.

My idea would be to proceed the following:

• Add the clang-format targets where they don't already exist
• Start reformatting the projects, I would prefer to start with smaller projects with few open MRs first. This wil of course require manual intervention in cases where clang-format would uglify the code.
• Start formmatting the bigger projects like kio or plasma-workspace. To avoid much clashing with open MRs we could format individual components like tests/applets/runners. But then we could not have a single commit we could use as a reference for git blame

Also the CMake files are sometimes horribly inconsistent, maybe we could utilize https://github.com/cheshirekow/cmake_format, but that is rather low priority.

@mart @davidedmundson @nicolasfella Feel fee to comment 😄

Doing KIO and other frameworks requires a discussion on the frameworks devel mailing list.

I don't think we need to wait for frameworks to start deploying on plasma.

Two things I am currently not 100% happy with:

• We wrap each lambda to multiple lines, even if they would fit on a single line, like https://invent.kde.org/frameworks/ktexteditor/-/commit/500024827b8d1dbdc4f0b901784c3537f8ddb15d#441630f8ea218920ba72b1e0fb0ffbbbf82d4d44_296_302 or https://invent.kde.org/network/kaccounts-integration/-/merge_requests/18/diffs#f3eb0d4b751fdb152e187714aa403f01f00f084e_61_65. There is the Inline key for the AllowShortLambdasOnASingleLine option, but IMHO that is too aggressive and would put too complex lambdas on a single line. We could modify the maximum length per line, but that seems like not a good idea either.

• Same goes for some short functions, like getters that just return a member variable, currently they would get wrapped onto multiple lines. And IMO that looks bad and wastes a bunch of lines, an example where this is heavily used is yakuake/app/skin.h

I wish we could say that if the lambda/method fits on a single line it should not be modified, only if it is too long it should be wrapped.

Excluding some sections from the formatting will of course still be an option. What do others think about this?

I think consistency trumps all things.

I don't think we should start to exclude stuff, that just will lead to "yeah, I don't it for my 2 files, exclude it".

If you look at KTextEditor, I don't think it does horrible stuff.

If there are a few minor tweaks to be done for the default clang-format file, we can discuss that thought.

Btw., in AbsInt we use this file (or at least the base variant of it that I submitted initially) now since >> 5 years or so on a rather large code base and beside the not that nice collapsing of if (... || ... | ...) conditions, nothing that ugly happens.
(and it stands to reason if if-conditions that are more or less 10 lines long are a good idea anyways code wise)

If there are a few minor tweaks to be done for the default clang-format file, we can discuss that thought.

My idea was that we should settle for a config where most people can be happy with to avoid having to re-format entire projects with bigger changes ;)

But since we are pretty much happy with the current config we can utilize that.

As we have discussed in the plasma meeting a pre-commit hook would be very suitable to prevent unformatted code from being pushed. See https://invent.kde.org/frameworks/extra-cmake-modules/-/merge_requests/65 for a proposal.

But one more thing: clang-format can't take care of the parentheses, the KDE coding style say that one must set these. Even in if statements or for loops with one line. IMO it could make sense to run clang-tidy while we are reformatting the code.
Especially in old parts of the codebase that is done really inconsistent.

See https://stackoverflow.com/a/63324313

In T11214#243600, @akulichalexandr wrote:

Maybe we can use the upstream formatter? It is available since Qt 5.15.

https://code.qt.io/cgit/qt/qtdeclarative.git/tree/tools/qmlformat

This removed the curly braces if the if statement has only one line . I do not see any option to change this behavior.