Enforce 100 chars line width
AbandonedPublic

Authored by romangg on Oct 21 2019, 11:25 AM.

Details

Reviewers
cullmann
Group Reviewers
Frameworks
Summary

The KDE Frameworks style recommended a 100 chars line limit but did not enforce
it. Now with the clang-format file currently a somewhat arbitrary limit of 240
chars is enforced.

Instead enforce the recommendation of 100 chars per line.

Diff Detail

Repository
R240 Extra CMake Modules
Branch
columnLimit
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 17966
Build 17984: arc lint + arc unit
romangg created this revision.Oct 21 2019, 11:25 AM
Restricted Application added projects: Frameworks, Build System. · View Herald TranscriptOct 21 2019, 11:25 AM
Restricted Application added subscribers: kde-buildsystem, kde-frameworks-devel. · View Herald Transcript
romangg requested review of this revision.Oct 21 2019, 11:25 AM

As explained in the thread on https://phabricator.kde.org/T11214, this will make the formatting ugly as hell, as if you have long method names, stuff is broken in arbitrary bad ways.
I don't want to change that, if nobody can avoid the resulting breakage.

As explained in the thread on https://phabricator.kde.org/T11214, this will make the formatting ugly as hell, as if you have long method names, stuff is broken in arbitrary bad ways.
I don't want to change that, if nobody can avoid the resulting breakage.

Does it brutalize the line breaks always or only when the code line does not comply with the 100 chars limit? Or from the other side does ColumnLimit: 240 mean that all "unnecessary" manual line breaks will be removed such that there are >200 chars lines now where before a document was only with <200? This would be even worse.

It is unfortunately very brutal and in-deed, it can create very long lines if you have matching bad constructs.

One can play with stuff like a limit of 160 oder whatever to limit that, but as it brutally hard break and align stuff, with e.g. 100 for my tries, a lot of "OK" constructs that break this length got broken up in unreadable chunks.

We play a long time with this at company and settled for 240 as some long lines are less worse than a lot of lalal->lalalala->lalala->llalala-> sequences broken up in X lines.

Perhaps easiest way to see what happens: apply it to one of your things and vary the value.

zzag added a subscriber: zzag.Oct 21 2019, 12:55 PM

I suggest to set ColumnLimit to 0 by default and allow projects to override it.

I am a long-time advocate of columnLimits; however, in our modern world of programming I think 100 is too short.

240 may be a bit too long: in my personal coding style scripts I try to limit to 120. even 120 is hard to achieve sometimes.
I feel that 240 is a fair compromise.

-1 for this patch

See the other task: columnlimit 0 leads to endless long lines and is a unusable default.

As said above, I think 100 is a bad idea. Can we close this?

I agree. A limit of 100 is arbitrary and harmful. Unfortunately clang-format doesn't seem to have a way to make lines over a certain length less desirably, but still allowed if the break would be ugly, so it is better to just allow long lines.

romangg added a comment.EditedSep 24 2020, 9:16 PM

FYI I'm using a strict 100 chars limit nowadays on all my KWinFT projects (with the exception of KWinFT itself for now) and I'm very happy with this decision.

As an example you can compare these two otherwise still pretty somewhat similar files:
https://invent.kde.org/plasma/libkscreen/-/blob/master/src/backendmanager.cpp
https://gitlab.com/kwinft/disman/-/blob/master/lib/backendmanager.cpp

I don't even think about any of this anymore, especially I don't care if clang-format moves a function name or puts a bracket here or there. I just apply clang-format to the file and that's it. The consistency trumps anything else.

Anyways closing this diff as requested.

romangg abandoned this revision.Sep 24 2020, 9:16 PM