On Sat, Jan 2, 2021 at 2:07 AM David Edmundson
wrote:
>
> It does. I've tried to collect some stats and provide some context on
> the change.
>
> Taking just plasma-workspace, we have a lot of lines over 100
> characters at the moment.
> 3817 lines to be exact. ~3.3% [1]
> We also have 146 lines over 160 chars. One is 383 chars long!
This kind of number crunching is for sure helpful to form an informed
decision. Kudos!
> When the limit was 100 we had some feedback that we "break" those
> lines, so the clang-format change was made explicitly to address that.
> Especially as the existing framework policy is a "try to" not "you
> must", so there is an argument that an explicit limit has to be
> higher.
I get that argument but there are good reasons for choosing a
comparably low limit. Priority must be to ensure the readability of
the code. For continuous text this is around 60 chars [1]. While code
is of course different I would still argue it's comparable to a
certain degree as the eye movement needs to go over the full line
length too, especially if one reads some code for the first time. By
this comparison 100 chars is rather spacious, everything above
probably reduces the readability strongly.
On the other hand if such a limit "breaks" lines it is a good
indicator that this code should be refactored anyways as it will be
difficult to read for other people with or without the line breaks.
> We can't fulfill the conflicting goals of both not affecting existing
> code too much and simultaneously force the frameworks guideline as a
> strict rule.
I believe we both agree that if there is a certain limit it should be
a strict rule and not just a recommendation being ignored on a
case-by-case basis. This way enforcing the limit can be automated. I
do that in CI pipelines on all merge requests in my KWinFT projects.
Saves me from 99% of discussions about coding style and me forgetting
to run clang-format on one of my own commits.
> After running clang-format we get 4.0% of lines over 100 chars long,
> and none over 160 chars.
> So it does mean some lines get longer, but overall we're not talking
> about a huge amount of code.
This is interesting data but the "enforcement" is done only once and
afterwards influences all later code. So it should be argued
independent of the immediate change as the long-term effect is
prevalent.
> There is a MR on ECM for setting PenaltyBreakBeforeFirstCallParameter
> which tries to push more to the 100 chars, if that's preferred.
To me that looks like a workaround for a problem that shouldn't exist
in the first place.
Note that PenaltyBreakBeforeFirstCallParameter is not a value in chars
like MR69 claims but is a dimension-less penalty. See git's appliance
as a good example on how to use it [2]. By the way with a strict
column limit of 80.
> We can also explore updating the frameworks policy given how we've
> drifted apart from using it in our actual codebases.
Changing the Frameworks policy is a must if the official cmake-format
tooling would produce different code and it annoys me that I'm the
only one pointing that out. How can you recommend a certain style and
then provide tooling that will destroy it again even if a Frameworks
consumer previously made sure his code complies to that style?
But as said if this recommendation should indeed change from 100 chars
to something else is independent on how the tooling would "break" the
current code and instead should only depend on how well people can
read code at a certain line length and if it is possible to write C++
constructs comfortably with "only" 100 chars line length (it is). The
code must then be adapted to comply with that recommendation and not
the other way around.
[1] https://baymard.com/blog/line-length-readability
[2] https://github.com/git/git/blob/master/.clang-format#L175-L181
> David
>
> [1] find -name '*\.cpp' | xargs cat | awk '{print length}' | sort |
> uniq -c | awk '{print $2,$1}' | sort -n > line_length.csv
