Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1471247 > unrolled thread

checkkpatch (in)sanity ?

Back to article view | Back to linux.kernel

Contents

  checkkpatch (in)sanity ? Joe Perches <joe@perches.com> - 2016-08-27 22:50 +0200
    Re: checkkpatch (in)sanity ? "Levin, Alexander" <alexander.levin@verizon.com> - 2016-08-28 03:10 +0200
      Re: checkkpatch (in)sanity ? Joe Perches <joe@perches.com> - 2016-08-28 03:50 +0200
        Re: checkkpatch (in)sanity ? Joe Perches <joe@perches.com> - 2016-08-28 04:30 +0200
        Re: checkkpatch (in)sanity ? "Levin, Alexander" <alexander.levin@verizon.com> - 2016-08-28 04:50 +0200
          Re: checkkpatch (in)sanity ? Joe Perches <joe@perches.com> - 2016-08-28 19:20 +0200
            Re: checkkpatch (in)sanity ? Greg KH <gregkh@linuxfoundation.org> - 2016-08-28 20:00 +0200
            Re: checkkpatch (in)sanity ? "Levin, Alexander" <alexander.levin@verizon.com> - 2016-08-29 00:40 +0200
              Re: checkkpatch (in)sanity ? Joe Perches <joe@perches.com> - 2016-08-29 01:30 +0200
                Re: checkkpatch (in)sanity ? "Levin, Alexander" <alexander.levin@verizon.com> - 2016-08-29 04:30 +0200
                  Re: checkkpatch (in)sanity ? Christoph Hellwig <hch@infradead.org> - 2016-08-29 10:30 +0200
              Re: [Ksummit-discuss] checkkpatch (in)sanity ? Alexandre Belloni <alexandre.belloni@free-electrons.com> - 2016-08-29 09:20 +0200
                Re: [Ksummit-discuss] checkkpatch (in)sanity ? Arnd Bergmann <arnd@arndb.de> - 2016-08-29 11:10 +0200
                  Re: [Ksummit-discuss] checkkpatch (in)sanity ? Joe Perches <joe@perches.com> - 2016-08-29 14:50 +0200
                    Re: [Ksummit-discuss] checkkpatch (in)sanity ? Josh Triplett <josh@joshtriplett.org> - 2016-08-29 19:20 +0200
                      Re: [Ksummit-discuss] checkkpatch (in)sanity ? Joe Perches <joe@perches.com> - 2016-08-29 19:50 +0200
            RE: [Ksummit-discuss] checkkpatch (in)sanity ? "Luck, Tony" <tony.luck@intel.com> - 2016-08-29 19:50 +0200
              Re: [Ksummit-discuss] checkkpatch (in)sanity ? Joe Perches <joe@perches.com> - 2016-08-29 20:10 +0200
                Re: [Ksummit-discuss] checkkpatch (in)sanity ? Joe Perches <joe@perches.com> - 2016-08-29 20:50 +0200
                Re: [Ksummit-discuss] checkkpatch (in)sanity ? Josh Triplett <josh@joshtriplett.org> - 2016-08-29 21:10 +0200
                Re: [Ksummit-discuss] checkkpatch (in)sanity ? Arnd Bergmann <arnd@arndb.de> - 2016-08-29 23:10 +0200
        Re: checkkpatch (in)sanity ? Kalle Valo <kvalo@codeaurora.org> - 2016-08-29 13:20 +0200
          Re: checkkpatch (in)sanity ? Joe Perches <joe@perches.com> - 2016-08-29 14:40 +0200
            Re: checkkpatch (in)sanity ? Kalle Valo <kvalo@codeaurora.org> - 2016-08-29 20:10 +0200
              Re: checkkpatch (in)sanity ? Joe Perches <joe@perches.com> - 2016-08-29 21:10 +0200
              Re: [Ksummit-discuss] checkkpatch (in)sanity ? Arnd Bergmann <arnd@arndb.de> - 2016-08-29 23:10 +0200
      Re: [Ksummit-discuss] checkkpatch (in)sanity ? Alexey Dobriyan <adobriyan@gmail.com> - 2016-08-28 10:00 +0200
        Re: [Ksummit-discuss] checkkpatch (in)sanity ? Julia Lawall <julia.lawall@lip6.fr> - 2016-08-28 12:00 +0200
          Re: [Ksummit-discuss] checkkpatch (in)sanity ? Joe Perches <joe@perches.com> - 2016-08-28 22:00 +0200
            Re: [Ksummit-discuss] checkkpatch (in)sanity ? Jiri Kosina <jikos@kernel.org> - 2016-08-28 22:40 +0200
            Re: [Ksummit-discuss] checkkpatch (in)sanity ? Dennis Kaarsemaker <dennis@kaarsemaker.net> - 2016-08-28 23:30 +0200
              Re: [Ksummit-discuss] checkkpatch (in)sanity ? Joe Perches <joe@perches.com> - 2016-08-29 00:00 +0200
    Re: [Ksummit-discuss] checkkpatch (in)sanity ? Dan Carpenter <dan.carpenter@oracle.com> - 2016-08-29 21:10 +0200
      Re: [Ksummit-discuss] checkkpatch (in)sanity ? Dan Carpenter <dan.carpenter@oracle.com> - 2016-08-29 21:20 +0200
        Re: [Ksummit-discuss] checkkpatch (in)sanity ? Joe Perches <joe@perches.com> - 2016-08-29 21:40 +0200
      Re: [Ksummit-discuss] checkkpatch (in)sanity ? Josh Triplett <josh@joshtriplett.org> - 2016-08-29 21:20 +0200
        Re: [Ksummit-discuss] checkkpatch (in)sanity ? Joe Perches <joe@perches.com> - 2016-08-29 21:30 +0200

Page 1 of 2  [1] 2  Next page →

#1471247 — checkkpatch (in)sanity ?

On Fri, Aug 26, 2016 at 01:26:35PM +0200, Greg KH wrote:
> On Fri, Aug 26, 2016 at 12:46:51AM -0400, Levin, Alexander wrote:
>>    - Making checkpatch check for (some) of the stable kernel rules
>>    (and possibly recommend adding the stable@ tag in certain cases?).
>>      - Depends on: making checkpatch sane again.>This sounds interesting.  What do you mean by "sane"?

Sasha, can you expand your thoughts here please?

Most all of the trivial spacing stuff can easily be
ignored either by a human determining what's important
or by using command line options like --ignore=spacing

[toc] | [next] | [standalone]

#1471258

On Sat, Aug 27, 2016 at 04:40:52PM -0400, Joe Perches wrote:
> On Fri, Aug 26, 2016 at 01:26:35PM +0200, Greg KH wrote:
> > On Fri, Aug 26, 2016 at 12:46:51AM -0400, Levin, Alexander wrote:
> >>    - Making checkpatch check for (some) of the stable kernel rules
> >>    (and possibly recommend adding the stable@ tag in certain cases?).
> >>      - Depends on: making checkpatch sane again.>This sounds interesting.  What do you mean by "sane"?
> 
> Sasha, can you expand your thoughts here please?

Sure. I have 2.5 concerns about the state of checkpatch:

On Sat, Aug 27, 2016 at 04:40:52PM -0400, Joe Perches wrote: 
> Most all of the trivial spacing stuff can easily be
> ignored either by a human determining what's important
> or by using command line options like --ignore=spacing

1.
This is the wrong default. By default checkpatch shouldn't be showing trivial
issues that encourage folks to try and work around them and as a result
produce worse code.

Look at the 80 character limit warning for example, what good does it do? It
encourages people to do even stupider things to work around it and results in
a bunch of "fix checkpatch warning" that touch existing code just to make the
result harder to read and make 'git blame' harder to work with.

By default you should only get the most critical warnings we have in the
kernel like missing S-O-B or corrupt patch.


2. A "who wrote these rules?": there seems to be a disconnect between the rules
checkpatch is trying to enforce and the accepted coding style enforced by
maintainers. 

Do a git-format-patch on all of the commits Linus authored in the past year or
two and see how many of them fail checkpatch (or do the same for any of the
commits that passed through and were accepted by the top maintainers),
according to checkpatch we need to make those guys stop touching the kernel.


3. This one is somewhat subjective: scripts/checkpatch.pl is a massive blob of
perl code that a fair amount of people don't know how to deal with. In 4.8 it's
6142 lines, making it the 124th largest source file in the kernel, well within
the top 1% of source files in the kernel.

This combination of size/language pushes people away from being involved in
what is supposed to be a central tool and gives them a reason to never use
it again after they see results they don't agree with (rather than fixing it).

-- 

Thanks,
Sasha

[toc] | [prev] | [next] | [standalone]

#1471262

On Sat, 2016-08-27 at 21:06 -0400, Levin, Alexander wrote:
> On Sat, Aug 27, 2016 at 04:40:52PM -0400, Joe Perches wrote:
> > On Fri, Aug 26, 2016 at 01:26:35PM +0200, Greg KH wrote:
> > > On Fri, Aug 26, 2016 at 12:46:51AM -0400, Levin, Alexander wrote:
> > > > 
> > > >     - Making checkpatch check for (some) of the stable kernel rules
> > > >     (and possibly recommend adding the stable@ tag in certain cases?).
> > > >       - Depends on: making checkpatch sane again
> > > > >This sounds interesting.  What do you mean by "sane"?
> > Sasha, can you expand your thoughts here please?
> Sure. I have 2.5 concerns about the state of checkpatch:
[]
> > Most all of the trivial spacing stuff can easily be
> > ignored either by a human determining what's important
> > or by using command line options like --ignore=spacing
> 1.
> This is the wrong default. By default checkpatch shouldn't be showing trivial
> issues that encourage folks to try and work around them and as a result
> produce worse code.
> 
> Look at the 80 character limit warning for example, what good does it do?

That argument's been done several times. It keeps Linus happy.
I don't care one way or another.

I think the biggest issue is the seriousness that some people
take checkpatch messages as dicta instead of ignorable bleats.

I still think ERROR->defect, WARNING->unstylish, CHECK->nitpick
would be a good change.

https://lkml.org/lkml/2015/7/16/568

>  It
> encourages people to do even stupider things to work around it and results in
> a bunch of "fix checkpatch warning" that touch existing code just to make the
> result harder to read and make 'git blame' harder to work with.

Almost all of the crud in git-blame can be avoided with -w

> By default you should only get the most critical warnings we have in the
> kernel like missing S-O-B or corrupt patch.

I don't think so, but if you do, add a filter for ERROR only.

> 2. A "who wrote these rules?": there seems to be a disconnect between the rules
> checkpatch is trying to enforce and the accepted coding style enforced by
> maintainers. 

Name some please.

> Do a git-format-patch on all of the commits Linus authored in the past year or
> two and see how many of them fail checkpatch (or do the same for any of the
> commits that passed through and were accepted by the top maintainers),
> according to checkpatch we need to make those guys stop touching the kernel.

Try it yourself and tell me what's wrong with the messages:

$ git log --pretty=oneline --author=torvalds --no-merges --since=1-year-ago | \
  grep -v " Linux [34]" | \
  while read commit ; do \
    echo $commit ; \
    git log --stat -p -1 --format=email $(echo $commit | cut -f1 -d" ")  | \
      ./scripts/checkpatch.pl - ; \
  done

Here's a summary done with an additional

  grep -P "^(ERROR|WARNING)" | cut -f1,2 -d":" | \
  sort |uniq -c | sort -rn

     46 WARNING:LONG_LINE_COMMENT
     45 WARNING:LEADING_SPACE
     37 WARNING:LONG_LINE
     16 ERROR:GIT_COMMIT_ID
     11 WARNING:COMMIT_LOG_LONG_LINE
      5 WARNING:BRACES
      2 WARNING:BAD_SIGN_OFF
      2 WARNING:AVOID_BUG
      2 ERROR:SPACING
      1 WARNING:SPLIT_STRING
      1 WARNING:FILE_PATH_CHANGES
      1 WARNING:ENOSYS
      1 ERROR:MISSING_SIGN_OFF

> 3. This one is somewhat subjective: scripts/checkpatch.pl is a massive blob of
> perl code that a fair amount of people don't know how to deal with. In 4.8 it's
> 6142 lines, making it the 124th largest source file in the kernel, well within
> the top 1% of source files in the kernel.
> 
> This combination of size/language pushes people away from being involved in
> what is supposed to be a central tool and gives them a reason to never use
> it again after they see results they don't agree with (rather than fixing it).

Meh, I'm not a perl guy either.

I think almost all of it is regexes and most people
aren't very good at those.

So it wouldn't matter if it was perl or python.

spatch isn't the right tool.

What would you suggest instead?

[toc] | [prev] | [next] | [standalone]

#1471264

On Sat, 2016-08-27 at 18:42 -0700, Joe Perches wrote:

>      46 WARNING:LONG_LINE_COMMENT
>      45 WARNING:LEADING_SPACE
>      37 WARNING:LONG_LINE
>      16 ERROR:GIT_COMMIT_ID
>      11 WARNING:COMMIT_LOG_LONG_LINE
>       5 WARNING:BRACES
>       2 WARNING:BAD_SIGN_OFF
>       2 WARNING:AVOID_BUG
>       2 ERROR:SPACING
>       1 WARNING:SPLIT_STRING
>       1 WARNING:FILE_PATH_CHANGES
>       1 WARNING:ENOSYS
>       1 ERROR:MISSING_SIGN_OFF

My copy/pasta mistake, the above was Ingo Molnar's list

Here's Linus'

     37 WARNING:LONG_LINE
     19 ERROR:GIT_COMMIT_ID
      8 WARNING:PREFER_PR_LEVEL
      6 ERROR:SPACING
      4 WARNING:MACRO_WITH_FLOW_CONTROL
      3 WARNING:COMMIT_LOG_LONG_LINE
      2 WARNING:TYPO_SPELLING
      2 WARNING:BAD_SIGN_OFF
      2 ERROR:TRAILING_STATEMENTS
      1 WARNING:NEW_TYPEDEFS
      1 WARNING:LONG_LINE_COMMENT
      1 WARNING:LINE_SPACING
      1 WARNING:CONSTANT_COMPARISON
      1 WARNING:AVOID_BUG
      1 ERROR:STABLE_ADDRESS

[toc] | [prev] | [next] | [standalone]

#1471267

On Sat, Aug 27, 2016 at 09:42:59PM -0400, Joe Perches wrote:
> On Sat, 2016-08-27 at 21:06 -0400, Levin, Alexander wrote:
> > On Sat, Aug 27, 2016 at 04:40:52PM -0400, Joe Perches wrote:
> > > On Fri, Aug 26, 2016 at 01:26:35PM +0200, Greg KH wrote:
> > > > On Fri, Aug 26, 2016 at 12:46:51AM -0400, Levin, Alexander wrote:
> > > > > 
> > > > >     - Making checkpatch check for (some) of the stable kernel rules
> > > > >     (and possibly recommend adding the stable@ tag in certain cases?).
> > > > >       - Depends on: making checkpatch sane again
> > > > > >This sounds interesting.  What do you mean by "sane"?
> > > Sasha, can you expand your thoughts here please?
> > Sure. I have 2.5 concerns about the state of checkpatch:
> []
> > > Most all of the trivial spacing stuff can easily be
> > > ignored either by a human determining what's important
> > > or by using command line options like --ignore=spacing
> > 1.
> > This is the wrong default. By default checkpatch shouldn't be showing trivial
> > issues that encourage folks to try and work around them and as a result
> > produce worse code.
> > 
> > Look at the 80 character limit warning for example, what good does it do?
> 
> That argument's been done several times. It keeps Linus happy.
> I don't care one way or another.

I'm not trying to be specific with the 80 character thing, it's also true for
a few other things that makes people produce less readable code than what it
would have looked like if they'd ignore the warning.
 
> I think the biggest issue is the seriousness that some people
> take checkpatch messages as dicta instead of ignorable bleats.

That makes sense to you, but it doesn't make sense to the newer folks who are
told not to submit any patches with checkpatch errors/warnings. You know to
ignore these 80-character warnings when it makes sense, they see it as "you
must make the warning disappear no matter what".
 
> I still think ERROR->defect, WARNING->unstylish, CHECK->nitpick
> would be a good change.
> 
> https://lkml.org/lkml/2015/7/16/568

Probably. Would you agree that by default we shouldn't show anything that's
not an error/defect?

> >  It
> > encourages people to do even stupider things to work around it and results in
> > a bunch of "fix checkpatch warning" that touch existing code just to make the
> > result harder to read and make 'git blame' harder to work with.
> 
> Almost all of the crud in git-blame can be avoided with -w

That doesn't deal with newlines people add to hide the 80 character stuff, nor it
deals with the "harder to read" part.

> > By default you should only get the most critical warnings we have in the
> > kernel like missing S-O-B or corrupt patch.
> 
> I don't think so, but if you do, add a filter for ERROR only.

I could, but the problem is the people who see the default output as "holy".

> > 2. A "who wrote these rules?": there seems to be a disconnect between the rules
> > checkpatch is trying to enforce and the accepted coding style enforced by
> > maintainers. 
> 
> Name some please.

Well look at the git commit id SHA1 length thingie for example (GIT_COMMIT_ID). checkpatch says 12 chars minimum, but as far as I can tell Linus and Greg didn't get the memo.

> > Do a git-format-patch on all of the commits Linus authored in the past year or
> > two and see how many of them fail checkpatch (or do the same for any of the
> > commits that passed through and were accepted by the top maintainers),
> > according to checkpatch we need to make those guys stop touching the kernel.
> 
> Try it yourself and tell me what's wrong with the messages:
> 
> $ git log --pretty=oneline --author=torvalds --no-merges --since=1-year-ago | \
>   grep -v " Linux [34]" | \
>   while read commit ; do \
>     echo $commit ; \
>     git log --stat -p -1 --format=email $(echo $commit | cut -f1 -d" ")  | \
>       ./scripts/checkpatch.pl - ; \
>   done
> 
> Here's a summary done with an additional
> 
>   grep -P "^(ERROR|WARNING)" | cut -f1,2 -d":" | \
>   sort |uniq -c | sort -rn
> 
>      46 WARNING:LONG_LINE_COMMENT
>      45 WARNING:LEADING_SPACE
>      37 WARNING:LONG_LINE
>      16 ERROR:GIT_COMMIT_ID
>      11 WARNING:COMMIT_LOG_LONG_LINE
>       5 WARNING:BRACES
>       2 WARNING:BAD_SIGN_OFF
>       2 WARNING:AVOID_BUG
>       2 ERROR:SPACING
>       1 WARNING:SPLIT_STRING
>       1 WARNING:FILE_PATH_CHANGES
>       1 WARNING:ENOSYS
>       1 ERROR:MISSING_SIGN_OFF

$ git log --pretty=oneline --author=torvalds --no-merges --since=1-year-ago | grep -v " Linux [34]" | wc -l
64

Linus has more errors/warnings than commits. Why do we let him commit stuff?

> > 3. This one is somewhat subjective: scripts/checkpatch.pl is a massive blob of
> > perl code that a fair amount of people don't know how to deal with. In 4.8 it's
> > 6142 lines, making it the 124th largest source file in the kernel, well within
> > the top 1% of source files in the kernel.
> > 
> > This combination of size/language pushes people away from being involved in
> > what is supposed to be a central tool and gives them a reason to never use
> > it again after they see results they don't agree with (rather than fixing it).
> 
> Meh, I'm not a perl guy either.
> 
> I think almost all of it is regexes and most people
> aren't very good at those.
> 
> So it wouldn't matter if it was perl or python.
> 
> spatch isn't the right tool.
> 
> What would you suggest instead?

This is a good topic to talk about, making checkpatch accessible to us
commoners could be useful, we just need to figure out how.

-- 

Thanks,
Sasha

[toc] | [prev] | [next] | [standalone]

#1471404

On Sat, 2016-08-27 at 22:47 -0400, Levin, Alexander wrote:

> Would you agree that by default we shouldn't show anything that's
> not an error/defect?

Not particularly, no.

> That doesn't deal with newlines people add to hide the 80 character stuff, nor it
> deals with the "harder to read" part.

Harder to read is almost all habituation.

80 columns is simply silly when dealing with either
long identifiers or many levels of indentation.

One thing that 80 column limit does do is encourage
shorter identifiers and fewer levels of indentation.

Generally, both of those are good things.

> > > By default you should only get the most critical warnings we have in the
> > > kernel like missing S-O-B or corrupt patch.
> > I don't think so, but if you do, add a filter for ERROR only.
> I could, but the problem is the people who see the default output as "holy".

Personally, I think the "my first kernel patch" beginners were
overly encouraged to produce these checkpatch whitespace type
changes by a couple things:

o Greg KH's TuxRadar article back in 2010
  http://www.tuxradar.com/content/newbies-guide-hacking-linux-kernel
o The Eudyptula Challenge
  http://eudyptula-challenge.org/

I don't know if the Eudyptula scripts are specific to
drivers/staging and most of those beginners haven't read his
email from 2015 that essentially says "don't do that" on
anything other than drivers/staging.

https://lists.kernelnewbies.org/pipermail/kernelnewbies/2015-July/014699.html

Outreach is hard.  Those efforts were perhaps worthwhile, but
has even a single productive kernel developer been produced
from one of those two outreach efforts?

> > > 2. A "who wrote these rules?": there seems to be a disconnect between the rules
> > > checkpatch is trying to enforce and the accepted coding style enforced by
> > > maintainers. 
> > Name some please.
> Well look at the git commit id SHA1 length thingie for example (GIT_COMMIT_ID).
> checkpatch says 12 chars minimum, but as far as I can tell Linus and Greg didn't get the memo.

That bit is in Documentation/SubmttingPatches

12 was Linus' length after the original 7 was too short.
12 will still be long enough for a few years yet.
https://lkml.org/lkml/2010/10/28/287

> > I think almost all of it is regexes and most people
> > aren't very good at those.
> > 
> > So it wouldn't matter if it was perl or python.
> > 
> > spatch isn't the right tool.
> > 
> > What would you suggest instead?
> This is a good topic to talk about, making checkpatch accessible to us
> commoners could be useful, we just need to figure out how.

I'm not sure that matters much at all.

I'm sure if you tried, you could produce that
"ERRORS" only patch for checkpatch.

There would still be some issues categorizing the
various tests at the appropriate level to taste.

[toc] | [prev] | [next] | [standalone]

#1471424

On Sun, Aug 28, 2016 at 10:15:57AM -0700, Joe Perches wrote:
> On Sat, 2016-08-27 at 22:47 -0400, Levin, Alexander wrote:
> > > > By default you should only get the most critical warnings we have in the
> > > > kernel like missing S-O-B or corrupt patch.
> > > I don't think so, but if you do, add a filter for ERROR only.
> > I could, but the problem is the people who see the default output as "holy".
> 
> Personally, I think the "my first kernel patch" beginners were
> overly encouraged to produce these checkpatch whitespace type
> changes by a couple things:
> 
> o Greg KH's TuxRadar article back in 2010
>   http://www.tuxradar.com/content/newbies-guide-hacking-linux-kernel
> o The Eudyptula Challenge
>   http://eudyptula-challenge.org/
> 
> I don't know if the Eudyptula scripts are specific to
> drivers/staging and most of those beginners haven't read his
> email from 2015 that essentially says "don't do that" on
> anything other than drivers/staging.

I have been assured that Eudyptula says to stick only with
drivers/staging/  If anyone knows otherwise, please let me know and I
will work to resolve that.

thanks,

greg k-h

[toc] | [prev] | [next] | [standalone]

#1471485

On Sun, Aug 28, 2016 at 01:15:57PM -0400, Joe Perches wrote:
> On Sat, 2016-08-27 at 22:47 -0400, Levin, Alexander wrote:
> 
> > Would you agree that by default we shouldn't show anything that's
> > not an error/defect?
> 
> Not particularly, no.

I think that we need to figure out this disagreement first then. My claim is that checkpatch's output isn't useful.

Based on your bash snippet, populated with the KS program committee + the first few maintainers I spotted on 'git log':

commiter	commits		issues
arnd		858		2155
axboe		53		22
corbet		15		9
davem		55		81
grant.likely	2		0
gregkh		38 	 	46
hch 	 	393 	 	581
James.Bottomley	15 	 	15
martin.petersen	18 	 	20
mchehab 	678 		1042
mgorman 	104 		256
mingo 	 	58 		192
paulmck 	176 		68
peterz 	 	226 		511
rostedt 	123 		178
shuahkh 	53 		6
tglx 	 	200 		287
torvalds 	64 		89
tytso 		37 		77
viro 	 	350	 	256

And for the last 10,000 commits in the log, that script has observed 10,783 issues.

It'll be interesting to hear from these people about their view of checkpatch, but IMO when on average there are more issues than commits I can suggest two possible causes:

 1. People are used to ignore checkpatch warnings.
 2. People aren't using checkpatch.

Can you really make the claim that this is how checkpatch is supposed to be working?

-- 

Thanks,
Sasha

[toc] | [prev] | [next] | [standalone]

#1471491

On Sun, 2016-08-28 at 18:37 -0400, Levin, Alexander wrote:
> On Sun, Aug 28, 2016 at 01:15:57PM -0400, Joe Perches wrote:
> > On Sat, 2016-08-27 at 22:47 -0400, Levin, Alexander wrote:
> > > Would you agree that by default we shouldn't show anything that's
> > > not an error/defect?
> > Not particularly, no.
> I think that we need to figure out this disagreement first then. My
> claim is that checkpatch's output isn't useful.
[]
> It'll be interesting to hear from these people about their view of
> checkpatch, but IMO when on average there are more issues than commits
> I can suggest two possible causes:
> 
>  1. People are used to ignore checkpatch warnings.
>  2. People aren't using checkpatch.
> 
> Can you really make the claim that this is how checkpatch is supposed
> to be working?

<shrug>.  I make no particular claims about checkpatch.

I think checkpatch isn't particularly useful for those
thoroughly inculcated in what style the kernel uses and
is more useful for infrequent or new submitters.

The long time submitters and key maintainers are already
pretty consistent about coding style.

It would be good to examine the specific messages though.

For instance, Thomas Gleixner's messages for 200 commits:

     81 WARNING:COMMIT_LOG_LONG_LINE
     71 WARNING:BAD_SIGN_OFF
     37 WARNING:LONG_LINE
     22 WARNING:AVOID_BUG
     17 ERROR:SPACING
     16 WARNING:TYPO_SPELLING
     13 WARNING:UNSPECIFIED_INT
      7 ERROR:GIT_COMMIT_ID
      6 WARNING:MINMAX
      2 WARNING:PREFER_PR_LEVEL
      2 WARNING:LONG_LINE_COMMENT
      2 WARNING:LEADING_SPACE
      2 WARNING:FILE_PATH_CHANGES
      2 ERROR:CODE_INDENT
      1 WARNING:SUSPECT_CODE_INDENT
      1 WARNING:SPACING
      1 WARNING:ONE_SEMICOLON
      1 WARNING:LINE_SPACING
      1 WARNING:BLOCK_COMMENT_STYLE
      1 ERROR:TRAILING_STATEMENTS
      1 ERROR:INITIALISED_STATIC
      1 CHECK:PARENTHESIS_ALIGNMENT

28 ERRORs and a lot more WARNINGs

It seems that most of the BAD_SIGN_OFF uses are where
he signed his original patches and then did something
like git-am -s on those same patches.

The COMMIT_LOG_LONG_LINE messages are almost all just
slightly over the generic 80 column output when used
with just "git log".

I think the rest of the messages are reasonable and
generally follow CodingStyle.

[toc] | [prev] | [next] | [standalone]

#1471516

On Sun, Aug 28, 2016 at 07:20:52PM -0400, Joe Perches wrote:
> On Sun, 2016-08-28 at 18:37 -0400, Levin, Alexander wrote:
> > On Sun, Aug 28, 2016 at 01:15:57PM -0400, Joe Perches wrote:
> > > On Sat, 2016-08-27 at 22:47 -0400, Levin, Alexander wrote:
> > > > Would you agree that by default we shouldn't show anything that's
> > > > not an error/defect?
> > > Not particularly, no.
> > I think that we need to figure out this disagreement first then. My
> > claim is that checkpatch's output isn't useful.
> []
> > It'll be interesting to hear from these people about their view of
> > checkpatch, but IMO when on average there are more issues than commits
> > I can suggest two possible causes:
> > 
> >  1. People are used to ignore checkpatch warnings.
> >  2. People aren't using checkpatch.
> > 
> > Can you really make the claim that this is how checkpatch is supposed
> > to be working?
> 
> <shrug>.  I make no particular claims about checkpatch.
> 
> I think checkpatch isn't particularly useful for those
> thoroughly inculcated in what style the kernel uses and
> is more useful for infrequent or new submitters.
> 
> The long time submitters and key maintainers are already
> pretty consistent about coding style.

I did the same test for authors of 5-9 commits (just an arbitrary choice of numbers for "infrequent"), the results there are much worse: 3981 commits, 7175 issues.

The only big subsystem that seems to be forcing checkpatch "correctness" is mm/, where akpm is fixing up checkpatch issues himself. Otherwise, it looks like maintainers are not running checkpatch nor are making sure that the commits they merge in don't have checkpatch issues.

> It would be good to examine the specific messages though.

What for? The point is that with that amount of issues it's evident that people don't actually use checkpatch to begin with. We can discuss whether the output it produces makes sense all we want, but the fact is that people just don't use it - and I've tried to give my opinion of why I think it happens.

-- 

Thanks,
Sasha

[toc] | [prev] | [next] | [standalone]

#1471635

Seriously folks, checkpatch is a tool that's to be used for a reason,
not a reason by itself.

And it used to be a lot more useful before adding all kinds of bullshit
warnings.  I use checkpatch a lot, and I also ignore silly warnings in
it a lot as it's piling up more and more crap.

And then again there are just plenty of commits that touch existing code
for a minor change, and I'm not going to bother rewriting the steaming
pile of crap around it.  And checkpath really shouldn't even bother to
warn for context of diffs that's not even touched by default.

[toc] | [prev] | [next] | [standalone]

#1471590 — Re: [Ksummit-discuss] checkkpatch (in)sanity ?

On 28/08/2016 at 18:37:59 -0400, Levin, Alexander via Ksummit-discuss wrote :
> On Sun, Aug 28, 2016 at 01:15:57PM -0400, Joe Perches wrote:
> > On Sat, 2016-08-27 at 22:47 -0400, Levin, Alexander wrote:
> > 
> > > Would you agree that by default we shouldn't show anything that's
> > > not an error/defect?
> > 
> > Not particularly, no.
> 
> I think that we need to figure out this disagreement first then. My claim is that checkpatch's output isn't useful.
> 
> Based on your bash snippet, populated with the KS program committee + the first few maintainers I spotted on 'git log':
> 
> commiter	commits		issues
> arnd		858		2155
> axboe		53		22
> corbet		15		9
> davem		55		81
> grant.likely	2		0
> gregkh		38 	 	46
> hch 	 	393 	 	581
> James.Bottomley	15 	 	15
> martin.petersen	18 	 	20
> mchehab 	678 		1042
> mgorman 	104 		256
> mingo 	 	58 		192
> paulmck 	176 		68
> peterz 	 	226 		511
> rostedt 	123 		178
> shuahkh 	53 		6
> tglx 	 	200 		287
> torvalds 	64 		89
> tytso 		37 		77
> viro 	 	350	 	256
> 
> And for the last 10,000 commits in the log, that script has observed 10,783 issues.
> 
> It'll be interesting to hear from these people about their view of checkpatch, but IMO when on average there are more issues than commits I can suggest two possible causes:
> 
>  1. People are used to ignore checkpatch warnings.
>  2. People aren't using checkpatch.
> 

Well, Arnd is used to move around old code when refactoring. As the code
just moves, he rarely solves checkpatch issues when doing so which is
the right thing to do.

-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[toc] | [prev] | [next] | [standalone]

#1471660 — Re: [Ksummit-discuss] checkkpatch (in)sanity ?

On Monday, August 29, 2016 9:15:15 AM CEST Alexandre Belloni wrote:
> > 
> > commiter      commits         issues
> > arnd          858             2155
> > axboe         53              22
> > corbet                15              9
> > davem         55              81
> > grant.likely  2               0
> > gregkh                38              46
> > hch           393             581
> > James.Bottomley       15              15
> > martin.petersen       18              20
> > mchehab       678             1042
> > mgorman       104             256
> > mingo                 58              192
> > paulmck       176             68
> > peterz                226             511
> > rostedt       123             178
> > shuahkh       53              6
> > tglx          200             287
> > torvalds      64              89
> > tytso                 37              77
> > viro          350             256
> > 
> > And for the last 10,000 commits in the log, that script has observed 10,783 issues.
> > 
> > It'll be interesting to hear from these people about their view of checkpatch, but IMO when on average there are more issues than commits I can suggest two possible causes:
> > 
> >  1. People are used to ignore checkpatch warnings.
> >  2. People aren't using checkpatch.
> > 
> 
> Well, Arnd is used to move around old code when refactoring. As the code
> just moves, he rarely solves checkpatch issues when doing so which is
> the right thing to do.

I don't find checkpatch.pl overly useful for my own patches and rarely
run it. I looked over the last few hundred commits and found that almost
all the warnings were for:

- having overly long lines in commit messages when I quoted a long
  compiler warning. I generally don't wrap those to make it easier to
  search for the warnings in the git history

- missing the word "commit" before a reference to another changeset
  in full-text. I'll change that in the future if that makes people
  happy, but it doesn't seem important.

- existing style issues that I did not fix when fixing a bug. In many
  cases I find it better not to change coding style while fixing
  a bug, but there are other cases in which I do.

	Arnd

[toc] | [prev] | [next] | [standalone]

#1471803 — Re: [Ksummit-discuss] checkkpatch (in)sanity ?

On Mon, 2016-08-29 at 11:01 +0200, Arnd Bergmann wrote: 

> I don't find checkpatch.pl overly useful for my own patches and rarely
> run it.

I mostly run checkpatch to test new checkpatch rules.

I generally don't run it on my own patches, mostly out
of possibly misplaced confidence in my own adherence to
the nominal kernel style.  It sometimes leads to mild
regret over things like whitespace defects.

I get over it quickly.

But I also think checkpatch's overall false positive
reporting rate is relatively low.  Most all of what
it does to report possible defects is nominally correct.

If anyone has examples of bad reporting by checkpatch,
please send it.

[toc] | [prev] | [next] | [standalone]

#1471994 — Re: [Ksummit-discuss] checkkpatch (in)sanity ?

On Mon, Aug 29, 2016 at 05:47:59AM -0700, Joe Perches wrote:
> I generally don't run it on my own patches, mostly out
> of possibly misplaced confidence in my own adherence to
> the nominal kernel style.  It sometimes leads to mild
> regret over things like whitespace defects.

In the specific case of whitespace defects, does git diff not catch
those?

For that matter, should we add a .gitattributes file to the kernel
enabling additional whitespace errors git knows how to catch?  git.git
has such a file.

[toc] | [prev] | [next] | [standalone]

#1472010 — Re: [Ksummit-discuss] checkkpatch (in)sanity ?

On Mon, 2016-08-29 at 10:16 -0700, Josh Triplett wrote:
> On Mon, Aug 29, 2016 at 05:47:59AM -0700, Joe Perches wrote:
> > 
> > I generally don't run it on my own patches, mostly out
> > of possibly misplaced confidence in my own adherence to
> > the nominal kernel style.  It sometimes leads to mild
> > regret over things like whitespace defects.
> In the specific case of whitespace defects, does git diff not catch
> those?
> 
> For that matter, should we add a .gitattributes file to the kernel
> enabling additional whitespace errors git knows how to catch?  git.git
> has such a file.

For reference:
    https://git-scm.com/docs/gitattributes
https://git-scm.com/book/en/v2/Customizing-Git-Git-Configuration
and here's the git.git .gitattributes file:
    $ cat .gitattributes 
    * whitespace=!indent,trail,space
    *.[ch] whitespace=indent,trail,space diff=cpp
    *.sh whitespace=indent,trail,space

Using something like that for kernel git would be a
good idea for trailing whitespace and space before HT.

But aren't those the generic git default options now?

[toc] | [prev] | [next] | [standalone]

#1472012 — RE: [Ksummit-discuss] checkkpatch (in)sanity ?

> 80 columns is simply silly when dealing with either
> long identifiers or many levels of indentation.
>
> One thing that 80 column limit does do is encourage
> shorter identifiers and fewer levels of indentation.
>
> Generally, both of those are good things.

I think the main complaint with the limit is that people fix it by simply
breaking the long line, which often makes for less readable code.

Perhaps there would be less pushback on this if checkpatch also
complained about clumsily broken long lines and offered the advice
to restructure the code with helper functions etc. to avoid deep
indentation?

FWIW I do find checkpatch is helpful enough with useful tips
that it has value even when it generates some noise. Generally
the better you are at conforming to kernel style, the more irritating
it will be, because you only see the questionable output. For
newbies, and less frequent contributors (especially those who
work on other projects with other style guides) it is likely still
doing a good job.

In the journey from 4.6 to 4.7 we had 13433 commits. 2258 (16%)
from people with 5 or fewer commits in that release. Those are
the people most helped by checkpatch (plus the maintainers who
took those patches didn't have to spend as many cycles complaining
about style).

I think the bottom line is whether checkpatch's helpful messages do
more good than the grey area messages that cause people to make
questionable changes to shut checkpatch up.

-Tony

[toc] | [prev] | [next] | [standalone]

#1472035 — Re: [Ksummit-discuss] checkkpatch (in)sanity ?

On Mon, 2016-08-29 at 17:46 +0000, Luck, Tony wrote:
> > 
> > 80 columns is simply silly when dealing with either
> > long identifiers or many levels of indentation.
> > 
> > One thing that 80 column limit does do is encourage
> > shorter identifiers and fewer levels of indentation.
> > 
> > Generally, both of those are good things.
> I think the main complaint with the limit is that people fix it by simply
> breaking the long line, which often makes for less readable code.
> 
> Perhaps there would be less pushback on this if checkpatch also
> complained about clumsily broken long lines and offered the advice
> to restructure the code with helper functions etc. to avoid deep
> indentation?

It suggests that already for 6+ leading tabs, but some more
intelligence for nominally ugly added line breaks would
definitely help.

Using longish simple identifiers or multiple dereferences
can make the line breaks at 80 columns silly.

Simple things like:

			if (longish_identifier != AN_EVEN_LONGER_DEFINED_CONSTANT_VALUE)
and
			if (some_pointer->member[index].another_member >> shift_constant)

shouldn't really ever be broken into multiple lines,
but I see that submitted by some names I haven't seen
before all the time.

It's not an easy problem to solve with regexes though.

> FWIW I do find checkpatch is helpful enough with useful tips
> that it has value even when it generates some noise. Generally
> the better you are at conforming to kernel style, the more irritating
> it will be, because you only see the questionable output. For
> newbies, and less frequent contributors (especially those who
> work on other projects with other style guides) it is likely still
> doing a good job.
> 
> In the journey from 4.6 to 4.7 we had 13433 commits. 2258 (16%)
> from people with 5 or fewer commits in that release. Those are
> the people most helped by checkpatch (plus the maintainers who
> took those patches didn't have to spend as many cycles complaining
> about style).
> 
> I think the bottom line is whether checkpatch's helpful messages do
> more good than the grey area messages that cause people to make
> questionable changes to shut checkpatch up.
> 
> -Tony

[toc] | [prev] | [next] | [standalone]

#1472050 — Re: [Ksummit-discuss] checkkpatch (in)sanity ?

On Mon, 2016-08-29 at 11:01 -0700, Joe Perches wrote:
> On Mon, 2016-08-29 at 17:46 +0000, Luck, Tony wrote:
> > [checkpatch] offered the advice
> > to restructure the code with helper functions etc. to avoid deep
> > indentation?
> It suggests that already for 6+ leading tabs,

And here's an inexact little histogram of the code that
expands indent levels in the -next kernel source tree.

$ grep -rP --include=*.[ch] -oh "^[\t]+(do|while|for|if|else|return|goto|continue|switch|default|case|break)\b" * | \
  sed -r 's/^(\t+).*$/\1/' | awk '{print length($0)}' | sort -n | uniq -c
1217165 1
 783085 2
 249655 3
  59775 4
  11653 5
   1993 6
    444 7
    158 8
     50 9
     19 10
     10 11
      4 12
      1 13

Some of that code, as Linus once put it, is eye-gouging.
Luckily, almost all of the 7+ tab indent code is prehistoric.

[toc] | [prev] | [next] | [standalone]

#1472062 — Re: [Ksummit-discuss] checkkpatch (in)sanity ?

On Mon, Aug 29, 2016 at 11:01:40AM -0700, Joe Perches wrote:
> On Mon, 2016-08-29 at 17:46 +0000, Luck, Tony wrote:
> > > 
> > > 80 columns is simply silly when dealing with either
> > > long identifiers or many levels of indentation.
> > > 
> > > One thing that 80 column limit does do is encourage
> > > shorter identifiers and fewer levels of indentation.
> > > 
> > > Generally, both of those are good things.
> > I think the main complaint with the limit is that people fix it by simply
> > breaking the long line, which often makes for less readable code.
> > 
> > Perhaps there would be less pushback on this if checkpatch also
> > complained about clumsily broken long lines and offered the advice
> > to restructure the code with helper functions etc. to avoid deep
> > indentation?
> 
> It suggests that already for 6+ leading tabs, but some more
> intelligence for nominally ugly added line breaks would
> definitely help.
> 
> Using longish simple identifiers or multiple dereferences
> can make the line breaks at 80 columns silly.
> 
> Simple things like:
> 
> 			if (longish_identifier != AN_EVEN_LONGER_DEFINED_CONSTANT_VALUE)
> and
> 			if (some_pointer->member[index].another_member >> shift_constant)
> 
> shouldn't really ever be broken into multiple lines,

Agreed.

Honestly, I almost never see a line that should break solely based on
length.  Almost any line that makes sense to break at a given point
would make sense to break at that point even with a target line length
of 200.

For instance:

			if (an_interesting_function(x) == TARGET_VALUE_FOR_X
			    || an_interesting_function(y) == TARGET_VALUE_FOR_Y) {

That line break makes sense whether you want to break lines at 80
characters, 100, or 800.  (You could argue about
before-or-after-operator, or about line alignment.)  In almost no
circumstances would you want to also break around the '==', even though
that second line takes up 82 characters.

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web