linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] .clang-format: update column limit
@ 2020-06-10 12:51 Christian Brauner
  2020-06-10 15:55 ` Miguel Ojeda
  2020-06-10 17:13 ` Joe Perches
  0 siblings, 2 replies; 14+ messages in thread
From: Christian Brauner @ 2020-06-10 12:51 UTC (permalink / raw)
  To: Miguel Ojeda; +Cc: linux-kernel, clang-built-linux, Christian Brauner

The provided clang-format file wraps at 80 chars. If noone minds I'd like
to adjust this limit to 100 similar to what checkpatch (cf. [1]) uses now.

[1]: commit bdc48fa11e46 ("checkpatch/coding-style: deprecate 80-column warning")
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
 .clang-format | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/.clang-format b/.clang-format
index a0a96088c74f..2b314a14a658 100644
--- a/.clang-format
+++ b/.clang-format
@@ -52,7 +52,7 @@ BreakConstructorInitializersBeforeComma: false
 #BreakConstructorInitializers: BeforeComma # Unknown to clang-format-4.0
 BreakAfterJavaFieldAnnotations: false
 BreakStringLiterals: false
-ColumnLimit: 80
+ColumnLimit: 100
 CommentPragmas: '^ IWYU pragma:'
 #CompactNamespaces: false # Unknown to clang-format-4.0
 ConstructorInitializerAllOnOneLineOrOnePerLine: false

base-commit: abfbb29297c27e3f101f348dc9e467b0fe70f919
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH] .clang-format: update column limit
  2020-06-10 12:51 [PATCH] .clang-format: update column limit Christian Brauner
@ 2020-06-10 15:55 ` Miguel Ojeda
  2020-06-10 15:58   ` Nathan Chancellor
  2020-06-10 17:13 ` Joe Perches
  1 sibling, 1 reply; 14+ messages in thread
From: Miguel Ojeda @ 2020-06-10 15:55 UTC (permalink / raw)
  To: Christian Brauner; +Cc: linux-kernel, clang-built-linux

Hi Christian,

On Wed, Jun 10, 2020 at 2:51 PM Christian Brauner
<christian.brauner@ubuntu.com> wrote:
>
> The provided clang-format file wraps at 80 chars. If no one minds, I'd like
> to adjust this limit to 100 similar to what checkpatch (cf. [1]) uses now.

Thanks! Picking this up with a few changes to the commit message.

Cheers,
Miguel

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] .clang-format: update column limit
  2020-06-10 15:55 ` Miguel Ojeda
@ 2020-06-10 15:58   ` Nathan Chancellor
  2020-06-10 16:23     ` Sedat Dilek
  0 siblings, 1 reply; 14+ messages in thread
From: Nathan Chancellor @ 2020-06-10 15:58 UTC (permalink / raw)
  To: Miguel Ojeda; +Cc: Christian Brauner, linux-kernel, clang-built-linux

On Wed, Jun 10, 2020 at 05:55:14PM +0200, Miguel Ojeda wrote:
> Hi Christian,
> 
> On Wed, Jun 10, 2020 at 2:51 PM Christian Brauner
> <christian.brauner@ubuntu.com> wrote:
> >
> > The provided clang-format file wraps at 80 chars. If no one minds, I'd like
> > to adjust this limit to 100 similar to what checkpatch (cf. [1]) uses now.
> 
> Thanks! Picking this up with a few changes to the commit message.
> 
> Cheers,
> Miguel
> 

If it isn't too late:

Reviewed-by: Nathan Chancellor <natechancellor@gmail.com>

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] .clang-format: update column limit
  2020-06-10 15:58   ` Nathan Chancellor
@ 2020-06-10 16:23     ` Sedat Dilek
  0 siblings, 0 replies; 14+ messages in thread
From: Sedat Dilek @ 2020-06-10 16:23 UTC (permalink / raw)
  To: Nathan Chancellor
  Cc: Miguel Ojeda, Christian Brauner, linux-kernel, clang-built-linux

On Wed, Jun 10, 2020 at 5:58 PM Nathan Chancellor
<natechancellor@gmail.com> wrote:
>
> On Wed, Jun 10, 2020 at 05:55:14PM +0200, Miguel Ojeda wrote:
> > Hi Christian,
> >
> > On Wed, Jun 10, 2020 at 2:51 PM Christian Brauner
> > <christian.brauner@ubuntu.com> wrote:
> > >
> > > The provided clang-format file wraps at 80 chars. If no one minds, I'd like
> > > to adjust this limit to 100 similar to what checkpatch (cf. [1]) uses now.
> >
> > Thanks! Picking this up with a few changes to the commit message.
> >
> > Cheers,
> > Miguel
> >
>
> If it isn't too late:
>
> Reviewed-by: Nathan Chancellor <natechancellor@gmail.com>
>

Hi Christian,

just put your patch into my linux-5.7 patch-series before start building.

Feel free to add my:

Reviewed-by: Sedat Dilek <sedat.dilek@gmail.com>

Thanks.

Regards,
- Sedat -

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] .clang-format: update column limit
  2020-06-10 12:51 [PATCH] .clang-format: update column limit Christian Brauner
  2020-06-10 15:55 ` Miguel Ojeda
@ 2020-06-10 17:13 ` Joe Perches
  2020-06-10 17:32   ` Christian Brauner
  2020-06-11 10:03   ` Miguel Ojeda
  1 sibling, 2 replies; 14+ messages in thread
From: Joe Perches @ 2020-06-10 17:13 UTC (permalink / raw)
  To: Christian Brauner, Miguel Ojeda
  Cc: linux-kernel, clang-built-linux, Linus Torvalds

On Wed, 2020-06-10 at 14:51 +0200, Christian Brauner wrote:
> The provided clang-format file wraps at 80 chars. If noone minds I'd like
> to adjust this limit to 100 similar to what checkpatch (cf. [1]) uses now.
> 
> [1]: commit bdc48fa11e46 ("checkpatch/coding-style: deprecate 80-column warning")
> Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
[]
> diff --git a/.clang-format b/.clang-format
[]
> @@ -52,7 +52,7 @@ BreakConstructorInitializersBeforeComma: false
>  #BreakConstructorInitializers: BeforeComma # Unknown to clang-format-4.0
>  BreakAfterJavaFieldAnnotations: false
>  BreakStringLiterals: false
> -ColumnLimit: 80
> +ColumnLimit: 100

Ii think this is a not a good change.

If you read the commit log you provided, it ways
"staying withing 80 columns is certainly still _preferred_"

With this change, clang would _always_ wrap to 100 columns.

clang would not make any reasonable attempt to use 80 when
it should.



^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] .clang-format: update column limit
  2020-06-10 17:13 ` Joe Perches
@ 2020-06-10 17:32   ` Christian Brauner
  2020-06-11 10:03   ` Miguel Ojeda
  1 sibling, 0 replies; 14+ messages in thread
From: Christian Brauner @ 2020-06-10 17:32 UTC (permalink / raw)
  To: Joe Perches; +Cc: Miguel Ojeda, linux-kernel, clang-built-linux, Linus Torvalds

On Wed, Jun 10, 2020 at 10:13:24AM -0700, Joe Perches wrote:
> On Wed, 2020-06-10 at 14:51 +0200, Christian Brauner wrote:
> > The provided clang-format file wraps at 80 chars. If noone minds I'd like
> > to adjust this limit to 100 similar to what checkpatch (cf. [1]) uses now.
> > 
> > [1]: commit bdc48fa11e46 ("checkpatch/coding-style: deprecate 80-column warning")
> > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> []
> > diff --git a/.clang-format b/.clang-format
> []
> > @@ -52,7 +52,7 @@ BreakConstructorInitializersBeforeComma: false
> >  #BreakConstructorInitializers: BeforeComma # Unknown to clang-format-4.0
> >  BreakAfterJavaFieldAnnotations: false
> >  BreakStringLiterals: false
> > -ColumnLimit: 80
> > +ColumnLimit: 100
> 
> Ii think this is a not a good change.
> 
> If you read the commit log you provided, it ways
> "staying withing 80 columns is certainly still _preferred_"

I read it; that's why the "if noone minds" is there.

> 
> With this change, clang would _always_ wrap to 100 columns.
> 
> clang would not make any reasonable attempt to use 80 when
> it should.

You make it sounds like it caps all lines to 100 columns when really it
just does it for corner cases where we run over the 80 anwyways. I at
least don't regularly write lines of code that cross the 80 limit.
So when clang-format is called it's usually when something needs to be
reformatted at which point using the more lenient 100 char limit seems
sensible. But I don't particulary care about this patch. I can just
override the .clang-format file if this shakes the world too much.

Christian

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] .clang-format: update column limit
  2020-06-10 17:13 ` Joe Perches
  2020-06-10 17:32   ` Christian Brauner
@ 2020-06-11 10:03   ` Miguel Ojeda
  2020-06-11 10:36     ` Joe Perches
  1 sibling, 1 reply; 14+ messages in thread
From: Miguel Ojeda @ 2020-06-11 10:03 UTC (permalink / raw)
  To: Joe Perches
  Cc: Christian Brauner, linux-kernel, clang-built-linux, Linus Torvalds

Hi Joe,

On Wed, Jun 10, 2020 at 7:13 PM Joe Perches <joe@perches.com> wrote:
>
> Ii think this is a not a good change.
>
> If you read the commit log you provided, it ways
> "staying withing 80 columns is certainly still _preferred_"

Yes, but the related email discussions were not about establishing a
new hard limit, but about avoiding such hard limits for
historical/technical reasons.

> With this change, clang would _always_ wrap to 100 columns.

That is true, but it means clang-format will start splitting lines in
weird ways because it has to work with whatever the code is without
changes. If a programmer sees that the lines are too long after
applying the formatter, then it is a strong hint the code needs to be
rearranged somehow (e.g. creating intermediate results).

Hence keeping the limit at 80 means we will get strange arrangements
which could have fit in 100 just fine. In fact, I would argue a
*wider* limit is better for an automatic formatter, e.g. 120. Even
more so considering clang-format is not responsible for the final
formatting -- at least not yet :-) -- the developer is.

Cheers,
Miguel

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] .clang-format: update column limit
  2020-06-11 10:03   ` Miguel Ojeda
@ 2020-06-11 10:36     ` Joe Perches
  2020-06-11 11:54       ` Miguel Ojeda
  0 siblings, 1 reply; 14+ messages in thread
From: Joe Perches @ 2020-06-11 10:36 UTC (permalink / raw)
  To: Miguel Ojeda
  Cc: Christian Brauner, linux-kernel, clang-built-linux, Linus Torvalds

On Thu, 2020-06-11 at 12:03 +0200, Miguel Ojeda wrote:
> Hi Joe,
> 
> On Wed, Jun 10, 2020 at 7:13 PM Joe Perches <joe@perches.com> wrote:
> > Ii think this is a not a good change.
> > 
> > If you read the commit log you provided, it ways
> > "staying withing 80 columns is certainly still _preferred_"
> 
> Yes, but the related email discussions were not about establishing a
> new hard limit, but about avoiding such hard limits for
> historical/technical reasons.

Exactly.  So don't set a new hard limit of 100.

This would _always_ wrap lines to 100 columns when
80 columns is still preferred.

Imagine using a 100 column limit where a statement still
fits on 2 lines.  Now imagine the same statement wrapped
at 80 columns still fitting on 2 lines.

Which would you prefer and why?



^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] .clang-format: update column limit
  2020-06-11 10:36     ` Joe Perches
@ 2020-06-11 11:54       ` Miguel Ojeda
  2020-06-11 16:22         ` Joe Perches
  0 siblings, 1 reply; 14+ messages in thread
From: Miguel Ojeda @ 2020-06-11 11:54 UTC (permalink / raw)
  To: Joe Perches
  Cc: Christian Brauner, linux-kernel, clang-built-linux, Linus Torvalds

On Thu, Jun 11, 2020 at 12:36 PM Joe Perches <joe@perches.com> wrote:
>
> Exactly.  So don't set a new hard limit of 100.
>
> This would _always_ wrap lines to 100 columns when
> 80 columns is still preferred.

Why is 80 "still preferred" to begin with? The patch you sent for
`coding-style.rst` was picked up by Linus, yes, but the wording seems
too strong still, considering it is for newcomers.

The point is that 80 *isn't* a limit, so I don't see why it is
mentioned, much less "preferred". Rather, I would have worded it like
[*]. What do you think?

> Imagine using a 100 column limit where a statement still
> fits on 2 lines.  Now imagine the same statement wrapped
> at 80 columns still fitting on 2 lines.
>
> Which would you prefer and why?

The former. While sometimes it may be more aesthetically pleasing to
have 2 lines of similar lengths rather than a long one and a short
one, having a deterministic approach allows us to use automatic
formatters. Which in turn makes code more regular since breaks are
always done the same way (modulo heuristic differences between
clang-format versions etc.).

In other words, I prefer automatic breaks vs. discussing every break :-)

Cheers,
Miguel

[*] (please excuse any word-wrap)

From 3b3cad415b56498534fadf732f2762f4dbe108eb Mon Sep 17 00:00:00 2001
From: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>
Date: Thu, 11 Jun 2020 13:16:46 +0200
Subject: [PATCH] coding-style: don't mention line length hard limits, add tips

Signed-off-by: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>
---
 Documentation/process/coding-style.rst | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/Documentation/process/coding-style.rst
b/Documentation/process/coding-style.rst
index 17a8e584f15f..309d3ae17e6c 100644
--- a/Documentation/process/coding-style.rst
+++ b/Documentation/process/coding-style.rst
@@ -84,11 +84,13 @@ Get a decent editor and don't leave whitespace at
the end of lines.
 Coding style is all about readability and maintainability using commonly
 available tools.

-The preferred limit on the length of a single line is 80 columns.
+Avoid lines that are too long and use reasonable line lengths.  There is no
+hard limit: break lines where it makes the most sense, somewhere around
+the 80-120 columns.  Complex statements should be broken into sensible chunks;
+identifiers should not be unreasonably verbose.  Follow nearby conventions.

-Statements longer than 80 columns should be broken into sensible chunks,
-unless exceeding 80 columns significantly increases readability and does
-not hide information.
+A good test is using `clang-format`: if the formatter is unable to split
+the lines wisely, then the code likely needs rearrangement.

 Descendants are always substantially shorter than the parent and are
 are placed substantially to the right.  A very commonly used style
-- 
2.27.0

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH] .clang-format: update column limit
  2020-06-11 11:54       ` Miguel Ojeda
@ 2020-06-11 16:22         ` Joe Perches
  2020-06-11 18:51           ` Miguel Ojeda
  0 siblings, 1 reply; 14+ messages in thread
From: Joe Perches @ 2020-06-11 16:22 UTC (permalink / raw)
  To: Miguel Ojeda
  Cc: Christian Brauner, linux-kernel, clang-built-linux, Linus Torvalds

On Thu, 2020-06-11 at 13:54 +0200, Miguel Ojeda wrote:
> On Thu, Jun 11, 2020 at 12:36 PM Joe Perches <joe@perches.com> wrote:
> > Exactly.  So don't set a new hard limit of 100.
> > 
> > This would _always_ wrap lines to 100 columns when
> > 80 columns is still preferred.
> 
> Why is 80 "still preferred" to begin with?

That's neither my nor your issue to solve.

It does though underline why this patch
should not be applied.



^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] .clang-format: update column limit
  2020-06-11 16:22         ` Joe Perches
@ 2020-06-11 18:51           ` Miguel Ojeda
  2020-06-11 19:26             ` Joe Perches
  0 siblings, 1 reply; 14+ messages in thread
From: Miguel Ojeda @ 2020-06-11 18:51 UTC (permalink / raw)
  To: Joe Perches
  Cc: Christian Brauner, linux-kernel, clang-built-linux, Linus Torvalds

On Thu, Jun 11, 2020 at 6:22 PM Joe Perches <joe@perches.com> wrote:
>
> On Thu, 2020-06-11 at 13:54 +0200, Miguel Ojeda wrote:
> > Why is 80 "still preferred" to begin with?
>
> That's neither my nor your issue to solve.

? You (or Linus, I still don't know since the commit is on your name
but I can't find the full patch in the list...) updated the wording on
`coding-style.rst` and I have to decide what limit to put into
`ColumnLimit` to make `clang-format` most useful for developers within
the new constraints. So it does concern us...

In my view, `coding-style.rst` is saying what it used to say from the
point of view of a new contributor. But regardless of that, forcing
`clang-format` to work under the 80-column constraint means we will
get code that won't look as good as giving it a slightly higher hard
limit. Yes, more lines will get over 80, but that is not as big of a
concern now since it is not as "strongly preferred" as before, which
is the point I was trying to make.

A concern for keeping the `clang-format` limit as it is that I can
think of is that newcomers using `clang-format` may feel forced not to
go over 80-column no matter what since "that is what the tool does".
On the other hand, my concern for *increasing* it is about things like
function signatures, since there is no easy way to decrease their
length or rearrange them without reducing the identifiers' length. A
third alternative is using `ColumnWidth: 0` and let people break lines
on their own, `clang-format` will respect that if possible. But then
we rely on people using `checkpatch` and they have to take care of the
limit themselves, so the advantage of automatic formatting decreases.

By the way, I noticed that a lot of kernel code will still trigger the
100-column warning (~20% of the previous set triggering it!).

Cheers,
Miguel

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] .clang-format: update column limit
  2020-06-11 18:51           ` Miguel Ojeda
@ 2020-06-11 19:26             ` Joe Perches
  2020-06-22  0:03               ` Miguel Ojeda
  0 siblings, 1 reply; 14+ messages in thread
From: Joe Perches @ 2020-06-11 19:26 UTC (permalink / raw)
  To: Miguel Ojeda
  Cc: Christian Brauner, linux-kernel, clang-built-linux, Linus Torvalds

On Thu, 2020-06-11 at 20:51 +0200, Miguel Ojeda wrote:
> On Thu, Jun 11, 2020 at 6:22 PM Joe Perches <joe@perches.com> wrote:
> > On Thu, 2020-06-11 at 13:54 +0200, Miguel Ojeda wrote:
> > > Why is 80 "still preferred" to begin with?
> > 
> > That's neither my nor your issue to solve.
[]
> By the way, I noticed that a lot of kernel code will still trigger the
> 100-column warning (~20% of the previous set triggering it!).
> 
> Cheers,
> Miguel

Hey again Miguel:

A little script and some statistics:

Today about 6% of all code lines are > 80 columns

Oddities like the ~3% of all lines with 113 and 121 columns
are machine generated.

So realistically, < 3% of code is > 80 columns.

Separate out whatever subsystems you like on the first line:

$ git ls-files -- '*.[ch]' | \
  xargs expand | \
  awk '{ print length($0);}' | \
  sort -n | uniq -c | \
  awk '{array[NR] = $0; count[NR] = $1; sum += $1;} END { total = 0; for (i = 1; i <= NR; i++) { total += count[i]; printf "%s %.2f\n", array[i], total / sum * 100;}}'  
awk: cmd. line:1: (FILENAME=- FNR=4440107) warning: Invalid multibyte data detected. There may be a mismatch between your data and your locale.
3455770 0 13.23
1157486 1 17.66
 574694 2 19.86
 280390 3 20.93
  31446 4 21.05
  29545 5 21.17
  73717 6 21.45
   7345 7 21.48
  13018 8 21.53
 476703 9 23.35
 180246 10 24.04
 143531 11 24.59
  68625 12 24.85
  26514 13 24.95
  72634 14 25.23
  73967 15 25.51
 248466 16 26.47
 384157 17 27.94
 189412 18 28.66
 268797 19 29.69
 201788 20 30.46
 173988 21 31.13
 310914 22 32.32
 268757 23 33.35
 251725 24 34.31
 378764 25 35.76
 297660 26 36.90
 314712 27 38.10
 310217 28 39.29
 254886 29 40.27
 331904 30 41.54
 346239 31 42.86
 263004 32 43.87
 345515 33 45.19
 330351 34 46.46
 315555 35 47.67
 324772 36 48.91
 292872 37 50.03
 331934 38 51.30
 312446 39 52.50
 296784 40 53.63
 327897 41 54.89
 317866 42 56.10
 308287 43 57.28
 328277 44 58.54
 294287 45 59.67
 320852 46 60.90
 297384 47 62.03
 297185 48 63.17
 311184 49 64.36
 300313 50 65.51
 290202 51 66.62
 297554 52 67.76
 285576 53 68.85
 326253 54 70.10
 294160 55 71.23
 269516 56 72.26
 289752 57 73.37
 275687 58 74.43
 263747 59 75.44
 275174 60 76.49
 254685 61 77.46
 256117 62 78.44
 261010 63 79.44
 264508 64 80.46
 265899 65 81.47
 244046 66 82.41
 240534 67 83.33
 236809 68 84.23
 236241 69 85.14
 235128 70 86.04
 258164 71 87.03
 217702 72 87.86
 266741 73 88.88
 219092 74 89.72
 204094 75 90.50
 194298 76 91.25
 226122 77 92.11
 168129 78 92.75
 169807 79 93.40
 112483 80 93.83
  35581 81 93.97
  27556 82 94.08
  25829 83 94.18
  24490 84 94.27
  26327 85 94.37
  58577 86 94.59
  19372 87 94.67
  15753 88 94.73
  19481 89 94.80
  13913 90 94.86
  36455 91 95.00
  58113 92 95.22
   8095 93 95.25
  11709 94 95.29
   6739 95 95.32
   5893 96 95.34
   6817 97 95.37
   5830 98 95.39
   5285 99 95.41
   4815 100 95.43
   5381 101 95.45
   4000 102 95.47
   5510 103 95.49
  82742 104 95.80
   2980 105 95.81
   4189 106 95.83
   2637 107 95.84
   2341 108 95.85
  89130 109 96.19
  13856 110 96.24
   2155 111 96.25
   2342 112 96.26
 337926 113 97.55
 131165 114 98.06
   8816 115 98.09
   1746 116 98.10
  81835 117 98.41
   5505 118 98.43
   1572 119 98.44
   1572 120 98.44
 378537 121 99.89
   1353 122 99.90
   1873 123 99.90
   1597 124 99.91
   1468 125 99.92
   1119 126 99.92
    843 127 99.92
    878 128 99.93
    834 129 99.93
    686 130 99.93
    771 131 99.94
    646 132 99.94
    698 133 99.94
    756 134 99.94
    400 135 99.95
    535 136 99.95
    362 137 99.95
    303 138 99.95
   1785 139 99.96
    400 140 99.96
    336 141 99.96
    407 142 99.96
    281 143 99.96
    291 144 99.96
    418 145 99.97
    245 146 99.97
    255 147 99.97
    343 148 99.97
    196 149 99.97
    165 150 99.97
    269 151 99.97
    190 152 99.97
    206 153 99.97
    339 154 99.97
    168 155 99.97
    174 156 99.98
    221 157 99.98
    149 158 99.98
    262 159 99.98
    243 160 99.98
    127 161 99.98
    101 162 99.98
    161 163 99.98
    153 164 99.98
    190 165 99.98
    119 166 99.98
     74 167 99.98
    185 168 99.98
    128 169 99.98
     54 170 99.98
    114 171 99.98
    104 172 99.98
     51 173 99.98
    113 174 99.98
    227 175 99.99
     66 176 99.99
     50 177 99.99
     99 178 99.99
     35 179 99.99
    106 180 99.99
     57 181 99.99
     36 182 99.99
     44 183 99.99
    225 184 99.99
     26 185 99.99
     55 186 99.99
     56 187 99.99
    417 188 99.99
     26 189 99.99
    104 190 99.99
     50 191 99.99
     94 192 99.99
     37 193 99.99
     16 194 99.99
     19 195 99.99
     36 196 99.99
     13 197 99.99
     15 198 99.99
     28 199 99.99
     50 200 99.99
     16 201 99.99
     23 202 99.99
      8 203 99.99
     10 204 99.99
     39 205 99.99
     17 206 99.99
     32 207 99.99
     72 208 99.99
     14 209 99.99
     15 210 99.99
     24 211 99.99
      6 212 99.99
     12 213 99.99
     36 214 99.99
     12 215 99.99
     17 216 99.99
     33 217 99.99
      6 218 99.99
      8 219 99.99
     18 220 99.99
      5 221 99.99
      5 222 99.99
      9 223 99.99
      5 224 99.99
      8 225 99.99
      5 226 99.99
      4 227 99.99
     11 228 99.99
      4 229 99.99
      3 230 99.99
      1 231 99.99
     20 232 99.99
      8 233 99.99
      8 234 99.99
     12 235 99.99
      5 236 99.99
     13 237 99.99
      5 238 99.99
      4 239 99.99
     13 240 99.99
    275 241 100.00
      3 242 100.00
      5 243 100.00
      7 244 100.00
     11 245 100.00
     13 246 100.00
      3 247 100.00
      9 248 100.00
      3 249 100.00
      6 250 100.00
      4 251 100.00
      1 252 100.00
      3 253 100.00
      3 254 100.00
      8 255 100.00
      3 256 100.00
      1 257 100.00
      1 258 100.00
      1 259 100.00
      6 260 100.00
      1 261 100.00
      1 262 100.00
      3 263 100.00
      2 264 100.00
      3 265 100.00
      3 266 100.00
      4 267 100.00
     10 268 100.00
     16 269 100.00
    132 270 100.00
      8 271 100.00
      1 272 100.00
      5 273 100.00
      1 274 100.00
     20 275 100.00
      2 276 100.00
      9 277 100.00
      1 278 100.00
      1 279 100.00
      2 280 100.00
      1 281 100.00
      8 282 100.00
      4 283 100.00
      4 284 100.00
     10 285 100.00
      1 286 100.00
      5 287 100.00
      2 288 100.00
      2 289 100.00
      1 290 100.00
      5 291 100.00
      2 292 100.00
      1 293 100.00
      3 294 100.00
      3 295 100.00
      2 296 100.00
      1 297 100.00
      1 298 100.00
    550 299 100.00
      1 300 100.00
      7 301 100.00
      6 302 100.00
      8 303 100.00
      4 304 100.00
      4 305 100.00
      1 306 100.00
      3 308 100.00
      1 309 100.00
      3 310 100.00
      1 314 100.00
      1 315 100.00
      1 316 100.00
      1 317 100.00
      1 318 100.00
      2 319 100.00
      3 320 100.00
      4 321 100.00
      2 323 100.00
      2 326 100.00
     13 327 100.00
     10 328 100.00
      2 329 100.00
      2 330 100.00
      2 331 100.00
      1 332 100.00
      2 334 100.00
      3 337 100.00
      1 338 100.00
      1 340 100.00
      2 341 100.00
      2 343 100.00
      1 344 100.00
      2 345 100.00
      1 346 100.00
      1 347 100.00
      1 348 100.00
      1 350 100.00
      2 352 100.00
      3 353 100.00
      2 354 100.00
      1 356 100.00
      2 362 100.00
      1 363 100.00
      1 364 100.00
      2 365 100.00
      1 371 100.00
      2 372 100.00
      2 373 100.00
      1 383 100.00
      1 386 100.00
      1 394 100.00
      1 395 100.00
      2 397 100.00
      1 398 100.00
      1 400 100.00
      1 402 100.00
      3 405 100.00
      1 414 100.00
      2 418 100.00
      1 419 100.00
      1 422 100.00
      1 440 100.00
      2 441 100.00
      1 442 100.00
      1 444 100.00
      1 452 100.00
      1 462 100.00
      1 464 100.00
      1 479 100.00
      1 480 100.00
      1 481 100.00
      1 492 100.00
      1 497 100.00
      1 501 100.00
      1 502 100.00
      1 503 100.00
      1 504 100.00
      1 510 100.00
      1 511 100.00
      2 517 100.00
      1 531 100.00
      1 534 100.00
      1 535 100.00
      1 539 100.00
      1 543 100.00
      1 544 100.00
      1 590 100.00
      1 608 100.00
      1 631 100.00
      1 702 100.00
      1 704 100.00
      1 712 100.00
      1 730 100.00
      1 733 100.00
      1 784 100.00
      1 790 100.00
      1 809 100.00
      1 823 100.00
      1 880 100.00
      1 901 100.00
      1 905 100.00
      1 947 100.00
      1 979 100.00
      1 1034 100.00
      1 1052 100.00
      1 1061 100.00
      1 1204 100.00
      5 1214 100.00
      1 1215 100.00
      2 1319 100.00


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] .clang-format: update column limit
  2020-06-11 19:26             ` Joe Perches
@ 2020-06-22  0:03               ` Miguel Ojeda
  2023-12-18 10:18                 ` Alexander Potapenko
  0 siblings, 1 reply; 14+ messages in thread
From: Miguel Ojeda @ 2020-06-22  0:03 UTC (permalink / raw)
  To: Joe Perches
  Cc: Christian Brauner, linux-kernel, clang-built-linux, Linus Torvalds

Hi Joe,

I didn't forget about this -- I was playing the other day with
`ColumnLimit: 0` and the new options up to LLVM 11 to see what we
could do... See below.

On Thu, Jun 11, 2020 at 9:26 PM Joe Perches <joe@perches.com> wrote:
>
> Hey again Miguel:
>
> A little script and some statistics:
>
> Today about 6% of all code lines are > 80 columns
>
> Oddities like the ~3% of all lines with 113 and 121 columns
> are machine generated.
>
> So realistically, < 3% of code is > 80 columns.

The data you show is very useful, thanks! Of course, the current set
of lines tends to be < 80 columns given the previous strongly
preferred limit, but I would expect that % to grow in the future.

By the way, the 20% figure I mentioned was w.r.t. the previous set of
lines that were already over 80 columns (not w.r.t. the total set).

Regarding the changes: `ColumnLimit: 0` could be a good compromise
(which makes `clang-format` respect as much as possible the current
line breaks) to let developers have more leeway. They would still get
warnings from your `checkpatch` etc. On the other hand, making
formatting dependent on the previous state is not something I am a fan
of, particularly for long-term efforts to move to `clang-format`
"completely".

I guess users of `clang-format` should speak up (w.r.t. `ColumnLimit:
100`, `ColumnLimit: 0` or no change). At the end of the day, this is
still just a tool and not enforced, and they can still override it, so
if people start submitting code with 200-wide lines, well, they still
can :-) Otherwise, let's leave it as it is for the moment and see what
is the output of the script in, say, a year.

Cheers,
Miguel

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] .clang-format: update column limit
  2020-06-22  0:03               ` Miguel Ojeda
@ 2023-12-18 10:18                 ` Alexander Potapenko
  0 siblings, 0 replies; 14+ messages in thread
From: Alexander Potapenko @ 2023-12-18 10:18 UTC (permalink / raw)
  To: miguel.ojeda.sandonis
  Cc: christian.brauner, clang-built-linux, joe, linux-kernel,
	torvalds, Alexander Potapenko, Yury Norov


Hi folks,

this came up in another code review
(https://lore.kernel.org/lkml/CAG_fn=WcrNqV4burBRPZZwoBLwgia7kerZ8g2vV5spzWF=houQ@mail.gmail.com/),
so I thought maybe it's time to revisit changing of ColumnLimit for clang-format?

I ran the script Joe provided, and there's a noticeable drift towards the longer lines in the past
three years:

 201789 78 87.16
 201579 79 87.77
 138155 80 88.19
  55886 81 88.35
  42656 82 88.48
  39539 83 88.60
  36490 84 88.71
  37856 85 88.83
  68131 86 89.03
  29352 87 89.12
  23277 88 89.19
  26902 89 89.27
  21405 90 89.34
  44506 91 89.47
  70788 92 89.69
  13416 93 89.73
  16650 94 89.78
  10872 95 89.81
   9786 96 89.84
  12170 97 89.88
   9139 98 89.90
   8516 99 89.93
   7008 100 89.95
   6058 101 89.97
   4692 102 89.98

At our team we try to run clang-format on all newly written code, which to great extent helps
to avoid style-related bikeshedding. I changed the local limit in my checkout and reflowed
mm/kmsan/*.c and mm/kfence/*.c, and they actually look better than before.

This is sure not enough to justify the 80->100 change, but as far as I can tell right now
reviewers are not actively paying attention at the lines that are shorter than 100 columns,
even if those exceed the 80-column limit. So even if all clang-format users switch to 100 columns
locally this wouldn't cause objections as long as checkpatch.pl is happy.
On the other hand, the readability of the code they produce is likely to increase.

Given that nothing changes for people who don't use clang-format, maybe having the limit conform
to what checkpatch.pl wants is not a bad idea after all?

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2023-12-18 10:18 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-10 12:51 [PATCH] .clang-format: update column limit Christian Brauner
2020-06-10 15:55 ` Miguel Ojeda
2020-06-10 15:58   ` Nathan Chancellor
2020-06-10 16:23     ` Sedat Dilek
2020-06-10 17:13 ` Joe Perches
2020-06-10 17:32   ` Christian Brauner
2020-06-11 10:03   ` Miguel Ojeda
2020-06-11 10:36     ` Joe Perches
2020-06-11 11:54       ` Miguel Ojeda
2020-06-11 16:22         ` Joe Perches
2020-06-11 18:51           ` Miguel Ojeda
2020-06-11 19:26             ` Joe Perches
2020-06-22  0:03               ` Miguel Ojeda
2023-12-18 10:18                 ` Alexander Potapenko

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).