Re: Making changes to the Coding Style

Re: Making changes to the Coding Style

[Joe Perches]

(cc'ing lkml, Andrew Morton and Linus)

Hi David.

I'm making a few comments to your otherwise unedited
original email sent to me and ksummit-2013-discuss below:

On Mon, 2013-09-02 at 15:31 +0100, David Howells wrote:
> I have some questions about the process of changing the coding style:
>
>  (1) Should there be a procedure for changing the kernel coding style so that
>      people don't find out from checkpatch that what was fine yesterday now
>      gets you moaned at?
> 
>  (2) Where should changes be announced so that enough people see it that there
>      can be discussion?  I suggest that this not be LKML due to the amount of
>      noise.  Perhaps a kernel-announce list?

I do not read ksummit-2013-announce.
This should be sent to lkml as it's the widest list.
Perhaps other lists as well, but certainly lkml at a
minimum.

>  (3) Who maintains the coding style?  Who arbitrates since coding style is
>      very much subjective?
> 
>  (4) Do we actually need to make changes at all when people are generally okay
>      with what we already have?
> 
>  (5) To what extent should local conditions be allowed to prevail?  For
>      example, in CodingStyle, there are different commenting rules for net/
>      and drivers/net/.  There are also unwritten variations in how various
>      bits of the tree are styled.
> 
>  (6) If the coding style does get changed, should existing lines within the
>      kernel then be retroactively changed?
> 
> 
> To illustrate a recent case:
> 
> A patch was posted to LKML to have checkpatch require the removal of "extern"
> from all function prototypes added in patches.  Sadly, I don't manage to read
> much of LKML these days and didn't see the initial posting.  The first I heard
> about this was when patches came my way retroactively fixing up kernel sources
> for which I'm responsible - patches which were firmly NAK'd.
> 
> I posted an objection to the coding style change itself[*] but the patch went
> in to upstream checkpatch anyway.  The first I found out about that was when
> Fengguang's automated git commit compile checker started sending me messages
> about patches that had previously been okay in this regard.
> 
> [*] Note that in this case, I disagree with the suggestion that the extern is
>     just visual noise - to me it's a visual cue.  I grant, however, that this
>     is subjective.
> Does this mean that the checkpatch crew are now the arbiters of the coding
> style, and whatever they decide is best just goes?

I make no such claims.

I do prefer standardized styles and I don't much care
what style is used.  I believe the standardized styles
can habituate developer's reading speeds for the better
and can also reduce the defect rate in new code.

extern is used in .h files far fewer times in function
prototypes than not. (~2:1 in favor of no extern) in
the kernel tree.

> Can we just delete checkpatch entirely?
> 
> Is the CodingStyle file obsolete, since things get put into checkpatch but not
> into CodingStyle?
> 
> </rant>
> 
> David
> 

Re: [Ksummit-2013-discuss] Making changes to the Coding Style

[Josh Triplett]

On Mon, 2013-09-02 at 15:31 +0100, David Howells wrote:
> I have some questions about the process of changing the coding style:
>
>  (1) Should there be a procedure for changing the kernel coding style so that
>      people don't find out from checkpatch that what was fine yesterday now
>      gets you moaned at?

Yes; at a minimum, changes to checkpatch should have accompanying
changes to Documentation/CodingStyle adding new requirements.  I'll send
a patch momentarily adding a comment to that effect.

I would suggest that the maintainers of checkpatch should NAK any
patches adding new style rules without accompanying changes to
Documentation/CodingStyle documenting those rules.

>  (2) Where should changes be announced so that enough people see it that there
>      can be discussion?  I suggest that this not be LKML due to the amount of
>      noise.  Perhaps a kernel-announce list?

I don't think it makes sense to create subset lists for areas that
specific, and a general "interesting patches" list seems far too
subjective to not get drowned in random CCs from people who don't want
their patches lost in LKML noise.  I also suspect most of the
subscriptions to such a list would come from people interested in
bikeshedding, which seems counterproductive.  (Not suggesting that
changes shouldn't get wider review; they absolutely should.  But a
dedicated list for style changes seems likely to produce particularly
poor results.)

I suspect that a "file subscription" mechanism would prove more
effective.  Currently, people actually *responsible* for an area of the
kernel can put relevant patterns in MAINTAINERS to help ensure that they
see relevant patches.  patchwork.kernel.org already sees all kernel
patches sent to LKML and many other lists; I wonder how much work it
would take to enhance patchwork.kernel.org to let users add file
patterns for which they'd like any matching patch mails bounced to them
as though they were CCed?

In the meantime, you might consider subscribing to LKML and writing some
mail filters for subjects you care about.  I know several people who
systematically read subsets of LKML based on keyword filters.  For
instance, mails to LKML containing "rcu" tend to reach Paul McKenney,
CCed or not.

>  (3) Who maintains the coding style?  Who arbitrates since coding style is
>      very much subjective?

This is exactly the kind of area for which it helps to have a project
with a single top-level maintainer rather than a committee. :)

Anyone else "maintaining" CodingStyle could collect, group,
sanity-check, and forward patches, but I don't see how anyone could
sensibly claim to maintain it in the sense of making ACK/NAK judgement
calls.

>  (5) To what extent should local conditions be allowed to prevail?  For
>      example, in CodingStyle, there are different commenting rules for net/
>      and drivers/net/.  There are also unwritten variations in how various
>      bits of the tree are styled.

As little as possible; the point of CodingStyle is to avoid local style
rules.  The local style in net/ and drivers/net/ is already a wart;
let's avoid introducing more, and for any that already exist let's
consider carefully whether to document the unwritten variations or treat
them as bugs to fix.  Most of the time the latter seems like the right
answer.

>  (6) If the coding style does get changed, should existing lines within the
>      kernel then be retroactively changed?

Depends on the new style rule, how much benefit it provides, and how
much noise the change produces.  Ideally yes, but massive
checkpatch-based patches don't tend to go over well outside of staging.

In the case of extern, that seems like more of a "don't add any new
instances" rule than a "systematically purge existing instances" rule; I
don't see much benefit to removing existing instances, except as an
incidental part of making other changes in the same area.

checkpatch used to print the following warning about this when using
--file mode, but that got reverted in a later version; perhaps it should
return?

WARNING: Using --file mode. Please do not send patches to linux-kernel
that change whole existing files if you did not significantly change most
of the file for other reasons anyways or just wrote the file newly
from scratch. Pure code style patches have a significant cost in a
quickly changing code base like Linux because they cause rejects
with other changes.

- Josh Triplett

[PATCH] checkpatch: Add comment about updating Documentation/CodingStyle

[Josh Triplett]

Patches to checkpatch that add new style rules should also change
Documentation/CodingStyle to document those new style rules; add a
comment to that effect to the top of scripts/checkpatch.pl.

Signed-off-by: Josh Triplett <josh@joshtriplett.org>
---
 scripts/checkpatch.pl | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 2ee9eb7..ba65ea6 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -4,6 +4,10 @@
 # (c) 2007,2008, Andy Whitcroft <apw@uk.ibm.com> (new conditions, test suite)
 # (c) 2008-2010 Andy Whitcroft <apw@canonical.com>
 # Licensed under the terms of the GNU GPL License version 2
+#
+# This file does not define the kernel coding style; Documentation/CodingStyle
+# does.  If you add a new style test to this file, add the corresponding style
+# rule it enforces to Documentation/CodingStyle.
 
 use strict;
 use POSIX;
-- 
1.8.4.rc3

Re: [Ksummit-2013-discuss] [PATCH] checkpatch: Add comment about updating Documentation/CodingStyle

[Mauro Carvalho Chehab]

Em Mon, 2 Sep 2013 11:19:01 -0700
Josh Triplett <josh@joshtriplett.org> escreveu:

> Patches to checkpatch that add new style rules should also change
> Documentation/CodingStyle to document those new style rules; add a
> comment to that effect to the top of scripts/checkpatch.pl.

Well, you forgot to c/c LKML on this patch; I think that KS2013 is not the
proper list to review this patch ;)

> 
> Signed-off-by: Josh Triplett <josh@joshtriplett.org>
> ---
>  scripts/checkpatch.pl | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 2ee9eb7..ba65ea6 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -4,6 +4,10 @@
>  # (c) 2007,2008, Andy Whitcroft <apw@uk.ibm.com> (new conditions, test suite)
>  # (c) 2008-2010 Andy Whitcroft <apw@canonical.com>
>  # Licensed under the terms of the GNU GPL License version 2
> +#
> +# This file does not define the kernel coding style; Documentation/CodingStyle
> +# does.  If you add a new style test to this file, add the corresponding style
> +# rule it enforces to Documentation/CodingStyle.
>  

Agreed with that. I would also add another comment there: "in case of
conflicts between checkpatch.pl and Documentation/CodingStyle, the latter
takes precedence."

Anyway,

Acked-by: Mauro Carvalho Chehab <m.chehab@samsung.com>

>  use strict;
>  use POSIX;

Regard
-- 

Cheers,
Mauro

Re: [Ksummit-2013-discuss] [PATCH] checkpatch: Add comment about updating Documentation/CodingStyle

[Joe Perches]

On Mon, 2013-09-02 at 15:39 -0300, Mauro Carvalho Chehab wrote:
> Em Mon, 2 Sep 2013 11:19:01 -0700
> Josh Triplett <josh@joshtriplett.org> escreveu:
[]
> > +# This file does not define the kernel coding style; Documentation/CodingStyle
> > +# does.  If you add a new style test to this file, add the corresponding style
> > +# rule it enforces to Documentation/CodingStyle.

> Agreed with that.

I do not.

> I would also add another comment there: "in case of
> conflicts between checkpatch.pl and Documentation/CodingStyle, the latter
> takes precedence."

There are many checkpatch rules (like semicolons) that
are not in CodingStyle.

CodingStyle should not become some intensely detailed
document that specifies the "only one true way" to
write code.

I also think checkpatch should not be used by robots
to determine that patches are bad or unacceptable.

checkpatch is just a dumb little tool that has some
utility but as Dan Carpenter once said, "it's less
sentient than a squirrel".

People should always use their own taste before
relying on dumb tools.

Re: [Ksummit-2013-discuss] [PATCH] checkpatch: Add comment about updating Documentation/CodingStyle

[Mauro Carvalho Chehab]

Em Mon, 02 Sep 2013 11:59:27 -0700
Joe Perches <joe@perches.com> escreveu:

> On Mon, 2013-09-02 at 15:39 -0300, Mauro Carvalho Chehab wrote:
> > Em Mon, 2 Sep 2013 11:19:01 -0700
> > Josh Triplett <josh@joshtriplett.org> escreveu:
> []
> > > +# This file does not define the kernel coding style; Documentation/CodingStyle
> > > +# does.  If you add a new style test to this file, add the corresponding style
> > > +# rule it enforces to Documentation/CodingStyle.
> 
> > Agreed with that.
> 
> I do not.
> 
> > I would also add another comment there: "in case of
> > conflicts between checkpatch.pl and Documentation/CodingStyle, the latter
> > takes precedence."
> 
> There are many checkpatch rules (like semicolons) that
> are not in CodingStyle.

Well, document them there, please.

> CodingStyle should not become some intensely detailed
> document that specifies the "only one true way" to
> write code.

There will always be things that will be freed to the programmer to
use, but CodingStyle should reflect what is the coding style we're
adopting at the Kernel, or putting it on another way: what things
will make the patch to be rejected because of its style.

> I also think checkpatch should not be used by robots
> to determine that patches are bad or unacceptable.
> 
> checkpatch is just a dumb little tool that has some
> utility but as Dan Carpenter once said, "it's less
> sentient than a squirrel".
> 
> People should always use their own taste before
> relying on dumb tools.

That's easier said than done. There are lots of stupid changes that are
done by developers (like enforcing 80 cols whitespace breaks) just
because of the checkpatch warnings. That happens before those patches 
got sent to the ML's, as most people know that maintainers will curse 
them if the coding style is crap[1].

[1] BTW, most of the time that checkpatch complains, the code is
really crap.

In any case, Documentation/CodingStyle is the reference document that
maintainers and coders should use to know that a code is following the
style (and not checkpatch). So, it requires updates when new
CodingStyle enforcements are created.

In the specific example pointed by David, if "extern", will start
to become a bad word that should be avoided, that should be documented
there at CodingStyle, and checkpatch should just be the dumb monkey
that will check that.

Cheers,
Mauro

Re: [Ksummit-2013-discuss] [PATCH] checkpatch: Add comment about updating Documentation/CodingStyle

[Josh Triplett]

On Mon, Sep 02, 2013 at 11:59:27AM -0700, Joe Perches wrote:
> On Mon, 2013-09-02 at 15:39 -0300, Mauro Carvalho Chehab wrote:
> > Em Mon, 2 Sep 2013 11:19:01 -0700
> > Josh Triplett <josh@joshtriplett.org> escreveu:
> []
> > > +# This file does not define the kernel coding style; Documentation/CodingStyle
> > > +# does.  If you add a new style test to this file, add the corresponding style
> > > +# rule it enforces to Documentation/CodingStyle.
> 
> > Agreed with that.
> 
> I do not.
> 
> > I would also add another comment there: "in case of
> > conflicts between checkpatch.pl and Documentation/CodingStyle, the latter
> > takes precedence."
> 
> There are many checkpatch rules (like semicolons) that
> are not in CodingStyle.

It's a rule of thumb, not a mandate.  In *general*, checkpatch.pl should
not be enforcing style rules that aren't documented in CodingStyle.

> CodingStyle should not become some intensely detailed
> document that specifies the "only one true way" to
> write code.

Any rule that maintainers are likely to enforce on patches they review
should live in Documentation/CodingStyle; unwritten rules are a bad
idea.  Any rule that maintainers are *not* likely to enforce shouldn't
go in scripts/checkpatch.pl.

- Josh Triplett

Re: [Ksummit-2013-discuss] [PATCH] checkpatch: Add comment about updating Documentation/CodingStyle

[Guenter Roeck]

On 09/02/2013 12:50 PM, Josh Triplett wrote:
> On Mon, Sep 02, 2013 at 11:59:27AM -0700, Joe Perches wrote:
>> On Mon, 2013-09-02 at 15:39 -0300, Mauro Carvalho Chehab wrote:
>>> Em Mon, 2 Sep 2013 11:19:01 -0700
>>> Josh Triplett <josh@joshtriplett.org> escreveu:
>> []
>>>> +# This file does not define the kernel coding style; Documentation/CodingStyle
>>>> +# does.  If you add a new style test to this file, add the corresponding style
>>>> +# rule it enforces to Documentation/CodingStyle.
>>
>>> Agreed with that.
>>
>> I do not.
>>
>>> I would also add another comment there: "in case of
>>> conflicts between checkpatch.pl and Documentation/CodingStyle, the latter
>>> takes precedence."
>>
>> There are many checkpatch rules (like semicolons) that
>> are not in CodingStyle.
>
> It's a rule of thumb, not a mandate.  In *general*, checkpatch.pl should
> not be enforcing style rules that aren't documented in CodingStyle.
>

Oddly enough, the opposite is true as well. 3.1, spaces around binary
and ternary operators, is for example not enforced, presumably because
it would generate too many positives. Since I like that rule, I have
my private version of checkpatch.pl which does check for it. After all,
it _is_ a CodingStyle rule.

Guenter

>> CodingStyle should not become some intensely detailed
>> document that specifies the "only one true way" to
>> write code.
>
> Any rule that maintainers are likely to enforce on patches they review
> should live in Documentation/CodingStyle; unwritten rules are a bad
> idea.  Any rule that maintainers are *not* likely to enforce shouldn't
> go in scripts/checkpatch.pl.
>

[PATCH] checkpatch: Report missing spaces around trigraphs with --strict

[Joe Perches]

Spaces around trigraphs are specified by CodingStyle
but checkpatch is currently silent about them because
there are many current instances without them.

Make missing spaces around trigraphs a --strict message.

Signed-off-by: Joe Perches <joe@perches.com>
---
> Oddly enough, the opposite is true as well. 3.1, spaces around binary
> and ternary operators, is for example not enforced, presumably because
> it would generate too many positives. Since I like that rule, I have
> my private version of checkpatch.pl which does check for it. After all,
> it _is_ a CodingStyle rule.

 scripts/checkpatch.pl | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 9bb056c..bb34c11 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2817,7 +2817,7 @@ sub process {
 				\+=|-=|\*=|\/=|%=|\^=|\|=|&=|
 				=>|->|<<|>>|<|>|=|!|~|
 				&&|\|\||,|\^|\+\+|--|&|\||\+|-|\*|\/|%|
-				\?|:
+				\?:|\?|:
 			}x;
 			my @elements = split(/($ops|;)/, $opline);
 
@@ -3040,15 +3040,13 @@ sub process {
 					    	$ok = 1;
 					}
 
-					# Ignore ?:
-					if (($opv eq ':O' && $ca =~ /\?$/) ||
-					    ($op eq '?' && $cc =~ /^:/)) {
-					    	$ok = 1;
-					}
-
+					# messages are ERROR, but ?: are CHK
 					if ($ok == 0) {
-						if (ERROR("SPACING",
-							  "spaces required around that '$op' $at\n" . $hereptr)) {
+						my $msg_type = \&ERROR;
+						$msg_type = \&CHK if (($op eq '?:' || $op eq '?' || $op eq ':') && $ctx =~ /VxV/);
+
+						if (&{$msg_type}("SPACING",
+								 "spaces required around that '$op' $at\n" . $hereptr)) {
 							$good = rtrim($fix_elements[$n]) . " " . trim($fix_elements[$n + 1]) . " ";
 							if (defined $fix_elements[$n + 2]) {
 								$fix_elements[$n + 2] =~ s/^\s+//;

Re: [PATCH] checkpatch: Report missing spaces around trigraphs with --strict

[Josh Triplett]

On Mon, Sep 02, 2013 at 03:14:46PM -0700, Joe Perches wrote:
> Spaces around trigraphs are specified by CodingStyle
> but checkpatch is currently silent about them because
> there are many current instances without them.
> 
> Make missing spaces around trigraphs a --strict message.
> 
> Signed-off-by: Joe Perches <joe@perches.com>

Reviewed-by: Josh Triplett <josh@joshtriplett.org>

> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -2817,7 +2817,7 @@ sub process {
>  				\+=|-=|\*=|\/=|%=|\^=|\|=|&=|
>  				=>|->|<<|>>|<|>|=|!|~|
>  				&&|\|\||,|\^|\+\+|--|&|\||\+|-|\*|\/|%|
> -				\?|:
> +				\?:|\?|:
>  			}x;

While you're poking at this bit of code, would you mind looking at why
it gives a false positive for spaces around '*' on my recent patch at
http://mid.gmane.org/20130901234251.GB25057@leaf ?  It appears to
mistake the '*' of a pointer for a multiply.

Thanks,
Josh Triplett

Re: [PATCH] checkpatch: Report missing spaces around trigraphs with --strict

[Joe Perches]

> would you mind looking at why
> it gives a false positive for spaces around '*' on my recent patch at
> http://mid.gmane.org/20130901234251.GB25057@leaf ?  It appears to
> mistake the '*' of a pointer for a multiply.

Looks like checkpatch thinks this should be a multiplication.

Try this:
---
 scripts/checkpatch.pl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 9bb056c..e421b5e 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3005,7 +3005,7 @@ sub process {
 					 $op eq '*' or $op eq '/' or
 					 $op eq '%')
 				{
-					if ($ctx =~ /Wx[^WCE]|[^WCE]xW/) {
+					if ($ctx =~ /Wx[^WCEB]|[^WCE]xW/) {
 						if (ERROR("SPACING",
 							  "need consistent spacing around '$op' $at\n" . $hereptr)) {
 							$good = rtrim($fix_elements[$n]) . " " . trim($fix_elements[$n + 1]) . " ";

Re: [PATCH] checkpatch: Report missing spaces around trigraphs with --strict

[Josh Triplett]

On Mon, Sep 02, 2013 at 04:54:25PM -0700, Joe Perches wrote:
> > would you mind looking at why
> > it gives a false positive for spaces around '*' on my recent patch at
> > http://mid.gmane.org/20130901234251.GB25057@leaf ?  It appears to
> > mistake the '*' of a pointer for a multiply.
> 
> Looks like checkpatch thinks this should be a multiplication.
> 
> Try this:
> ---
>  scripts/checkpatch.pl | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 9bb056c..e421b5e 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -3005,7 +3005,7 @@ sub process {
>  					 $op eq '*' or $op eq '/' or
>  					 $op eq '%')
>  				{
> -					if ($ctx =~ /Wx[^WCE]|[^WCE]xW/) {
> +					if ($ctx =~ /Wx[^WCEB]|[^WCE]xW/) {
>  						if (ERROR("SPACING",
>  							  "need consistent spacing around '$op' $at\n" . $hereptr)) {
>  							$good = rtrim($fix_elements[$n]) . " " . trim($fix_elements[$n + 1]) . " ";
> 
> 

That patch does indeed fix the problem, thanks!

Tested-by: Josh Triplett <josh@joshtriplett.org>

Re: [Ksummit-2013-discuss] [PATCH] checkpatch: Add comment about updating Documentation/CodingStyle

[David Howells]

Josh Triplett <josh@joshtriplett.org> wrote:

> > There are many checkpatch rules (like semicolons) that
> > are not in CodingStyle.
> 
> It's a rule of thumb, not a mandate.  In *general*, checkpatch.pl should
> not be enforcing style rules that aren't documented in CodingStyle.

Except that it becomes a mandate when someone runs it automatically against
every one of your patches and then sends you an email for each patch it finds
a checkpatch niggle against...

David

Re: [Ksummit-2013-discuss] [PATCH] checkpatch: Add comment about updating Documentation/CodingStyle

[Joe Perches]

On Mon, 2013-09-02 at 21:50 +0100, David Howells wrote:
> Josh Triplett <josh@joshtriplett.org> wrote:
> 
> > > There are many checkpatch rules (like semicolons) that
> > > are not in CodingStyle.
> > 
> > It's a rule of thumb, not a mandate.  In *general*, checkpatch.pl should
> > not be enforcing style rules that aren't documented in CodingStyle.
> 
> Except that it becomes a mandate when someone runs it automatically against
> every one of your patches and then sends you an email for each patch it finds
> a checkpatch niggle against...

I think that any robot sending such checkpatch-only
emails should be disabled.

I know of 2 email robots.

Fengguang Wu's very useful build robot
sends out emails on build failures.
I think that's great.

Wang Shilong <wangshilong1991@gmail.com>
sent me an automated checkpatch email I
thought was not useful.

Does anyone know of other checkpatch robots?

Re: [Ksummit-2013-discuss] [PATCH] checkpatch: Add comment about updating Documentation/CodingStyle

[Shilong Wang]

2013/9/3 Joe Perches <joe@perches.com>:
> On Mon, 2013-09-02 at 21:50 +0100, David Howells wrote:
>> Josh Triplett <josh@joshtriplett.org> wrote:
>>
>> > > There are many checkpatch rules (like semicolons) that
>> > > are not in CodingStyle.
>> >
>> > It's a rule of thumb, not a mandate.  In *general*, checkpatch.pl should
>> > not be enforcing style rules that aren't documented in CodingStyle.
>>
>> Except that it becomes a mandate when someone runs it automatically against
>> every one of your patches and then sends you an email for each patch it finds
>> a checkpatch niggle against...

Agree with this..
But using checkpatch.pl, i found there are *so many* patches that have
warnings or errors.

As far as i know, patches with checkpatch.pl's errors should be
avoided  at least unless
there is a *bug* in checkpatch.pl!

>
> I think that any robot sending such checkpatch-only
> emails should be disabled.
>
> I know of 2 email robots.
>
> Fengguang Wu's very useful build robot
> sends out emails on build failures.
> I think that's great.
>
> Wang Shilong <wangshilong1991@gmail.com>
> sent me an automated checkpatch email I
> thought was not useful.

I am sorry if i give you any trouble, i have disabled it(in fact, it
only has run for a day!)

Thanks,
wang
>
> Does anyone know of other checkpatch robots?
>
>

Re: [Ksummit-2013-discuss] [PATCH] checkpatch: Add comment about updating Documentation/CodingStyle

[Josh Triplett]

On Tue, Sep 03, 2013 at 08:26:21AM +0800, Shilong Wang wrote:
> 2013/9/3 Joe Perches <joe@perches.com>:
> > Wang Shilong <wangshilong1991@gmail.com>
> > sent me an automated checkpatch email I
> > thought was not useful.
> 
> I am sorry if i give you any trouble, i have disabled it(in fact, it
> only has run for a day!)

I would suggest that you leave it running, but rather than sending mails
directly, have it prep the mails for you to send after manual review.
Do some careful scrutiny for false positives and cases where the change
would not improve the code, and use checkpatch's options to turn off
the more contentious warnings (like the 80-column warning).  Over time,
you'll develop a set of options that produce warnings people mostly
*want* to get notified about.

- Josh Triplett

Re: [Ksummit-2013-discuss] [PATCH] checkpatch: Add comment about updating Documentation/CodingStyle

[Fengguang Wu]

On Mon, Sep 02, 2013 at 05:36:03PM -0700, Josh Triplett wrote:
> On Tue, Sep 03, 2013 at 08:26:21AM +0800, Shilong Wang wrote:
> > 2013/9/3 Joe Perches <joe@perches.com>:
> > > Wang Shilong <wangshilong1991@gmail.com>
> > > sent me an automated checkpatch email I
> > > thought was not useful.
> > 
> > I am sorry if i give you any trouble, i have disabled it(in fact, it
> > only has run for a day!)
> 
> I would suggest that you leave it running, but rather than sending mails
> directly, have it prep the mails for you to send after manual review.
> Do some careful scrutiny for false positives and cases where the change
> would not improve the code, and use checkpatch's options to turn off
> the more contentious warnings (like the 80-column warning).  Over time,
> you'll develop a set of options that produce warnings people mostly
> *want* to get notified about.
 
Good suggestions! That's exactly what I'm trying to do. And Joe kindly
showed me the initial list of checkpatch error types suitable for auto
notification.

Coverage is good: the checkpatch robot iterates over every new commit
in the 300+ git trees I collected over time. Some maintainer trees are
skipped because they should already run the check.

Here is the list of reports sent in the last two weeks. They are private
emails directly sent to the commit author and committer.  So far I've not
received complaints on these unsolicited checkpatch reports.

 Aug 23  [netdev-next:master 200/301] WARNING: usb_free_urb(NULL) is safe this check is probably n
 Aug 23  [netdev-next:master 202/301] WARNING: usb_free_urb(NULL) is safe this check is probably n
 Aug 23  [linuxtv-media:master 321/499] ERROR: Unrecognized email address: 'Kyungmin Park <kyungmi
 Aug 23  [linuxtv-media:master 322/499] ERROR: Unrecognized email address: 'Kyungmin Park <kyungmi
 Aug 24  [xlnx:master-next 32/53] WARNING: unnecessary cast may hide bugs, see http://c-faq.com/ma
 Aug 28  [mmotm:master 473/483] WARNING: __func__ should be used instead of gcc specific __FUNCTIO
 Aug 28  [kvm:queue 13/14] ERROR: Unrecognized email address: 'Gleb Natapov @gleb@redhat.com>'
 Aug 29  [dhowells-fs:keys-devel 9/12] WARNING: labels should not be indented
 Aug 29  [jolsa-perf:perf/plugins2 14/20] WARNING: storage class should be at the beginning of the
 Aug 30  [nfs:testing 47/61] ERROR: Unrecognized email address: 'Trond Myklebust <Trond.Myklebust@
 Aug 31  [josef-btrfs:master 74/135] WARNING: kfree(NULL) is safe this check is probably not requi
 Aug 31  [jolsa-perf:perf/toggle6 6/8] WARNING: kfree(NULL) is safe this check is probably not req
 Sep 01  [jolsa-perf:perf/core_plugins 14/24] WARNING: storage class should be at the beginning of

Thanks,
Fengguang

Re: [Ksummit-2013-discuss] [PATCH] checkpatch: Add comment about updating Documentation/CodingStyle

[Fengguang Wu]

On Mon, Sep 02, 2013 at 02:11:36PM -0700, Joe Perches wrote:
> On Mon, 2013-09-02 at 21:50 +0100, David Howells wrote:
> > Josh Triplett <josh@joshtriplett.org> wrote:
> > 
> > > > There are many checkpatch rules (like semicolons) that
> > > > are not in CodingStyle.
> > > 
> > > It's a rule of thumb, not a mandate.  In *general*, checkpatch.pl should
> > > not be enforcing style rules that aren't documented in CodingStyle.
> > 
> > Except that it becomes a mandate when someone runs it automatically against
> > every one of your patches and then sends you an email for each patch it finds
> > a checkpatch niggle against...
> 
> I think that any robot sending such checkpatch-only
> emails should be disabled.
> 
> I know of 2 email robots.
> 
> Fengguang Wu's very useful build robot
> sends out emails on build failures.
> I think that's great.

Thanks! Yes I'm now running checkpatch these days because some people
suggested to me that some of the checkpatch warnings do help catch
real bugs.

However I do try to avoid upsetting people with maybe-subjective
warnings. A checkpatch report will only be sent when a small fraction
of error types are detected. Comments are very welcome on how to
improve this list:

MEMSET
IN_ATOMIC
UAPI_INCLUDE
MALFORMED_INCLUDE       
SIZEOF_ADDRESS  
KREALLOC_ARG_REUSE      
EXECUTE_PERMISSIONS     
ERROR:BAD_SIGN_OFF      
LO_MACRO
HI_MACRO
CSYNC
SSYNC
HOTPLUG_SECTION
INDENTED_LABEL
INLINE_LOCATION
STORAGE_CLASS
USLEEP_RANGE
UNNECESSARY_CASTS
ALLOC_SIZEOF_STRUCT
KREALLOC_ARG_REUSE
USE_FUNC
LOCKDEP
EXPORTED_WORLD_WRITABLE
WHITESPACE_AFTER_LINE_CONTINUATION
MISSING_VMLINUX_SYMBOL
NEEDLESS_IF
PRINTF_L

Once the decision is made to send a checkpatch error/warning, the
report email will use the triggering error (the one that matters) as
the email subject, with the complete output of checkpatch.pl included
in email body.

Thanks,
Fengguang

Re: [Ksummit-2013-discuss] [PATCH] checkpatch: Add comment about updating Documentation/CodingStyle

[Joe Perches]

On Tue, 2013-09-03 at 08:39 +0800, Fengguang Wu wrote:
> On Mon, Sep 02, 2013 at 02:11:36PM -0700, Joe Perches wrote:
[]
> > Fengguang Wu's very useful build robot
> > sends out emails on build failures.
> > I think that's great.
> 
> Thanks! Yes I'm now running checkpatch these days because some people
> suggested to me that some of the checkpatch warnings do help catch
> real bugs.

Hi Fengguang.

I see, I don't recall receiving one of these so
it must be working just fine.

> However I do try to avoid upsetting people with maybe-subjective
> warnings. A checkpatch report will only be sent when a small fraction
> of error types are detected. Comments are very welcome on how to
> improve this list:

Your list seems reasonable.

I might add:

DOS_LINE_ENDINGS
MODIFIED_INCLUDE_ASM
JIFFIES_COMPARISON
ONE_SEMICOLON

Re: [Ksummit-2013-discuss] [PATCH] checkpatch: Add comment about updating Documentation/CodingStyle

[Fengguang Wu]

On Mon, Sep 02, 2013 at 05:47:54PM -0700, Joe Perches wrote:
> On Tue, 2013-09-03 at 08:39 +0800, Fengguang Wu wrote:
> > On Mon, Sep 02, 2013 at 02:11:36PM -0700, Joe Perches wrote:
> []
> > > Fengguang Wu's very useful build robot
> > > sends out emails on build failures.
> > > I think that's great.
> > 
> > Thanks! Yes I'm now running checkpatch these days because some people
> > suggested to me that some of the checkpatch warnings do help catch
> > real bugs.
> 
> Hi Fengguang.
> 
> I see, I don't recall receiving one of these so
> it must be working just fine.

Hi Joe!

Log shows that one of your patch being checked earlier today:

[4 days ago, Joe Perches] perf: Convert kmalloc_node(...GFP_ZERO...) to kzalloc_node()

If you have more patches in some git tree that missed the check,
please let me know.

> > However I do try to avoid upsetting people with maybe-subjective
> > warnings. A checkpatch report will only be sent when a small fraction
> > of error types are detected. Comments are very welcome on how to
> > improve this list:
> 
> Your list seems reasonable.
> 
> I might add:
> 
> DOS_LINE_ENDINGS
> MODIFIED_INCLUDE_ASM
> JIFFIES_COMPARISON
> ONE_SEMICOLON

Yeah they all look good to have. Thanks for the suggestions again!

Thanks,
Fengguang

Re: [Ksummit-2013-discuss] [PATCH] checkpatch: Add comment about updating Documentation/CodingStyle

[Josh Triplett]

On Tue, Sep 03, 2013 at 08:39:58AM +0800, Fengguang Wu wrote:
> On Mon, Sep 02, 2013 at 02:11:36PM -0700, Joe Perches wrote:
> > On Mon, 2013-09-02 at 21:50 +0100, David Howells wrote:
> > > Josh Triplett <josh@joshtriplett.org> wrote:
> > > 
> > > > > There are many checkpatch rules (like semicolons) that
> > > > > are not in CodingStyle.
> > > > 
> > > > It's a rule of thumb, not a mandate.  In *general*, checkpatch.pl should
> > > > not be enforcing style rules that aren't documented in CodingStyle.
> > > 
> > > Except that it becomes a mandate when someone runs it automatically against
> > > every one of your patches and then sends you an email for each patch it finds
> > > a checkpatch niggle against...
> > 
> > I think that any robot sending such checkpatch-only
> > emails should be disabled.
> > 
> > I know of 2 email robots.
> > 
> > Fengguang Wu's very useful build robot
> > sends out emails on build failures.
> > I think that's great.
> 
> Thanks! Yes I'm now running checkpatch these days because some people
> suggested to me that some of the checkpatch warnings do help catch
> real bugs.
> 
> However I do try to avoid upsetting people with maybe-subjective
> warnings. A checkpatch report will only be sent when a small fraction
> of error types are detected. Comments are very welcome on how to
> improve this list:
> 
> MEMSET
> IN_ATOMIC
> UAPI_INCLUDE
> MALFORMED_INCLUDE       
> SIZEOF_ADDRESS  
> KREALLOC_ARG_REUSE      
> EXECUTE_PERMISSIONS     
> ERROR:BAD_SIGN_OFF      
> LO_MACRO
> HI_MACRO
> CSYNC
> SSYNC
> HOTPLUG_SECTION
> INDENTED_LABEL
> INLINE_LOCATION
> STORAGE_CLASS
> USLEEP_RANGE
> UNNECESSARY_CASTS
> ALLOC_SIZEOF_STRUCT
> KREALLOC_ARG_REUSE
> USE_FUNC
> LOCKDEP
> EXPORTED_WORLD_WRITABLE
> WHITESPACE_AFTER_LINE_CONTINUATION
> MISSING_VMLINUX_SYMBOL
> NEEDLESS_IF
> PRINTF_L

Looks like you have KREALLOC_ARG_REUSE in that list twice.

Other than that, those look sensible.  I'd suggest a couple more, which
*should* always make sense, and to the best of my knowledge don't tend
to generate false positives:

C99_COMMENTS
CONFIG_EXPERIMENTAL
CVS_KEYWORD
ELSE_AFTER_BRACE
GLOBAL_INITIALIZERS
INITIALISED_STATIC
INVALID_UTF8
LINUX_VERSION_CODE
MISSING_EOF_NEWLINE
PREFER_SEQ_PUTS
PRINTK_WITHOUT_KERN_LEVEL
REDUNDANT_CODE
RETURN_PARENTHESES
SIZEOF_PARENTHESIS
SPACE_BEFORE_TAB
TRAILING_SEMICOLON
TRAILING_WHITESPACE
USE_DEVICE_INITCALL
USE_RELATIVE_PATH

These *ought* to make sense, but I don't know their false positive rates:

HEXADECIMAL_BOOLEAN_TEST
ALLOC_ARRAY_ARGS
CONSIDER_KSTRTO
CONST_STRUCT
SPLIT_STRING

The following almost always make sense, but only on patches not
yet applied to a tree:

PATCH_PREFIX
MODIFIED_INCLUDE_ASM
CORRUPTED_PATCH
NOT_UNIFIED_DIFF
MISSING_SIGN_OFF

- Josh Triplett

Re: [Ksummit-2013-discuss] [PATCH] checkpatch: Add comment about updating Documentation/CodingStyle

[Joe Perches]

On Mon, 2013-09-02 at 18:34 -0700, Josh Triplett wrote:
> I'd suggest a couple more, which
> *should* always make sense, and to the best of my knowledge don't tend
> to generate false positives:
> 
> C99_COMMENTS

I don't have a problem with c99 comments.
As far as I know, Linus doesn't either.

https://lkml.org/lkml/2012/4/16/473

> CONFIG_EXPERIMENTAL
> CVS_KEYWORD

OK, but <shrug>

> ELSE_AFTER_BRACE

I wouldn't do this one.  I think
there are some false positives here.

> GLOBAL_INITIALIZERS
> INITIALISED_STATIC

Nor these.

> INVALID_UTF8
> LINUX_VERSION_CODE
> MISSING_EOF_NEWLINE

OK I suppose.

> PREFER_SEQ_PUTS
> PRINTK_WITHOUT_KERN_LEVEL

There are a lot of these.
I suggest no here.

> RETURN_PARENTHESES
> SIZEOF_PARENTHESIS

It's in coding style, but some newish patches
do avoid them.  It's a question about how noisy
you want your robot to be.

> SPACE_BEFORE_TAB
> TRAILING_SEMICOLON
> TRAILING_WHITESPACE
> USE_DEVICE_INITCALL

> USE_RELATIVE_PATH

Having checkpatch tell people how to write changelogs
I think not a great idea.

> These *ought* to make sense, but I don't know their false positive rates:
> 
> HEXADECIMAL_BOOLEAN_TEST

That's a good one.  0 false positives.

> ALLOC_ARRAY_ARGS

Yes, this would be reasonable too.

> CONSIDER_KSTRTO

I think orobably not.  This would be a cleanup thing.

> CONST_STRUCT

OK

> SPLIT_STRING

I suggest no but <shrug>

Re: [Ksummit-2013-discuss] [PATCH] checkpatch: Add comment about updating Documentation/CodingStyle

[Josh Triplett]

On Mon, Sep 02, 2013 at 06:52:45PM -0700, Joe Perches wrote:
> On Mon, 2013-09-02 at 18:34 -0700, Josh Triplett wrote:
> > I'd suggest a couple more, which
> > *should* always make sense, and to the best of my knowledge don't tend
> > to generate false positives:
> > 
> > C99_COMMENTS
> 
> I don't have a problem with c99 comments.
> As far as I know, Linus doesn't either.
> 
> https://lkml.org/lkml/2012/4/16/473

That doesn't look like an endorsement so much as a statement that C99
comments are less awful than the net/ special-case comment style.

Documentation/CodingStyle chapter 8 says:
> Linux style for comments is the C89 "/* ... */" style.
> Don't use C99-style "// ..." comments.

If that no longer holds true, we should remove it from CodingStyle.  As
far as I know, though, it still holds.  In any case, it rarely comes up;
most kernel code doesn't use such comments.

> > CONFIG_EXPERIMENTAL
> > CVS_KEYWORD
> 
> OK, but <shrug>

Sure, I don't expect them to come up often.

> > ELSE_AFTER_BRACE
> 
> I wouldn't do this one.  I think
> there are some false positives here.

Oh?  What kinds of false positives have you seen?

In any case, fair enough.

> > GLOBAL_INITIALIZERS
> > INITIALISED_STATIC
> 
> Nor these.

I don't see an obvious way for those to have false positives.  What have
you seen?

> > INVALID_UTF8
> > LINUX_VERSION_CODE
> > MISSING_EOF_NEWLINE
> 
> OK I suppose.

Not particularly critical, but uncontroversial and no false positives.

> > PREFER_SEQ_PUTS
> > PRINTK_WITHOUT_KERN_LEVEL
> 
> There are a lot of these.
> I suggest no here.

I assume the bot only applies this to new patches, not to existing code,
in which case these seem completely reasonable.  New code should follow
these, even if we don't mass-fix existing code.

> > RETURN_PARENTHESES
> > SIZEOF_PARENTHESIS
> 
> It's in coding style, but some newish patches
> do avoid them.  It's a question about how noisy
> you want your robot to be.

These two seem reasonable to enforce on new code.  I agree that they
shouldn't trigger mass cleanups of existing code.

> > SPACE_BEFORE_TAB
> > TRAILING_SEMICOLON
> > TRAILING_WHITESPACE
> > USE_DEVICE_INITCALL

I didn't see any comment from you on these four.  Thoughts?

> > USE_RELATIVE_PATH
> 
> Having checkpatch tell people how to write changelogs
> I think not a great idea.

In general, sure, but that particular one seems OK.  In any case, not
particularly critical.

> > These *ought* to make sense, but I don't know their false positive rates:
> > 
> > HEXADECIMAL_BOOLEAN_TEST
> 
> That's a good one.  0 false positives.

Ah, good.

> > ALLOC_ARRAY_ARGS
> 
> Yes, this would be reasonable too.

Excellent.

> > CONSIDER_KSTRTO
> 
> I think orobably not.  This would be a cleanup thing.

Even if applied to new code only?  New code should use the right
functions to start with.

> > CONST_STRUCT
> 
> OK

Good to know; glad to hear it doesn't have false positives.

> > SPLIT_STRING
> 
> I suggest no but <shrug>

I can easily believe that it has too many false positives.  Let's leave
that one alone for now.

- Josh Triplett

Re: [Ksummit-2013-discuss] [PATCH] checkpatch: Add comment about updating Documentation/CodingStyle

[Joe Perches]

On Mon, 2013-09-02 at 19:12 -0700, Josh Triplett wrote:
> On Mon, Sep 02, 2013 at 06:52:45PM -0700, Joe Perches wrote:
> > On Mon, 2013-09-02 at 18:34 -0700, Josh Triplett wrote:
> > > I'd suggest a couple more, which
> > > *should* always make sense, and to the best of my knowledge don't tend
> > > to generate false positives:

Hey Josh.

I don't want to enable too many types of messages
because the "barrier to entry" to submit patches
shouldn't be so high that it discourages people.

I feel mostly that many types of checkpatch messages
are OK to emit, but aren't necessary to fix and
people should feel that checkpatch isn't a necessary
thing to silence before patches are accepted.

cheers, Joe

Re: [Ksummit-2013-discuss] [PATCH] checkpatch: Add comment about updating Documentation/CodingStyle

[Fengguang Wu]

On Mon, Sep 02, 2013 at 06:52:45PM -0700, Joe Perches wrote:
> On Mon, 2013-09-02 at 18:34 -0700, Josh Triplett wrote:
> > I'd suggest a couple more, which
> > *should* always make sense, and to the best of my knowledge don't tend
> > to generate false positives:
> > 
> > C99_COMMENTS
> 
> I don't have a problem with c99 comments.
> As far as I know, Linus doesn't either.
> 
> https://lkml.org/lkml/2012/4/16/473
> 
> > CONFIG_EXPERIMENTAL
> > CVS_KEYWORD
> 
> OK, but <shrug>
> 
> > ELSE_AFTER_BRACE
> 
> I wouldn't do this one.  I think
> there are some false positives here.
> 
> > GLOBAL_INITIALIZERS
> > INITIALISED_STATIC
> 
> Nor these.
> 
> > INVALID_UTF8
> > LINUX_VERSION_CODE
> > MISSING_EOF_NEWLINE
> 
> OK I suppose.
> 
> > PREFER_SEQ_PUTS
> > PRINTK_WITHOUT_KERN_LEVEL
> 
> There are a lot of these.
> I suggest no here.
> 
> > RETURN_PARENTHESES
> > SIZEOF_PARENTHESIS
> 
> It's in coding style, but some newish patches
> do avoid them.  It's a question about how noisy
> you want your robot to be.

I'd prefer the robot to show up only when necessary. The coding style
warnings are good for the developers who actively run checkpatch.pl to
make their patch better. However most are probably not suitable for a
robot to send people unsolicited warnings.

> > SPACE_BEFORE_TAB
> > TRAILING_SEMICOLON
> > TRAILING_WHITESPACE
> > USE_DEVICE_INITCALL
> 
> > USE_RELATIVE_PATH
> 
> Having checkpatch tell people how to write changelogs
> I think not a great idea.
> 
> > These *ought* to make sense, but I don't know their false positive rates:
> > 
> > HEXADECIMAL_BOOLEAN_TEST
> 
> That's a good one.  0 false positives.
> 
> > ALLOC_ARRAY_ARGS
> 
> Yes, this would be reasonable too.
> 
> > CONSIDER_KSTRTO
> 
> I think orobably not.  This would be a cleanup thing.

Perhaps we can run it for a while, so that people at least come to
aware there is a kstrto() for use. :)

> > CONST_STRUCT
> 
> OK
> 
> > SPLIT_STRING
> 
> I suggest no but <shrug>

Thanks for both of your suggestions! I'll add the commonly agreed ones:

+INVALID_UTF8
+LINUX_VERSION_CODE
+MISSING_EOF_NEWLINE
+HEXADECIMAL_BOOLEAN_TEST
+ALLOC_ARRAY_ARGS
+CONST_STRUCT
+CONSIDER_KSTRTO

And remove the duplicate one (good catch, Josh!)

-KREALLOC_ARG_REUSE

Thanks,
Fengguang

Re: [Ksummit-2013-discuss] [PATCH] checkpatch: Add comment about updating Documentation/CodingStyle

[Josh Triplett]

On Tue, Sep 03, 2013 at 10:46:40AM +0800, Fengguang Wu wrote:
> On Mon, Sep 02, 2013 at 06:52:45PM -0700, Joe Perches wrote:
> > On Mon, 2013-09-02 at 18:34 -0700, Josh Triplett wrote:
> > > CONFIG_EXPERIMENTAL
> > > CVS_KEYWORD
> > 
> > OK, but <shrug>
[...]
> Thanks for both of your suggestions! I'll add the commonly agreed ones:
> 
> +INVALID_UTF8
> +LINUX_VERSION_CODE
> +MISSING_EOF_NEWLINE
> +HEXADECIMAL_BOOLEAN_TEST
> +ALLOC_ARRAY_ARGS
> +CONST_STRUCT
> +CONSIDER_KSTRTO
> 
> And remove the duplicate one (good catch, Josh!)
> 
> -KREALLOC_ARG_REUSE

You missed CONFIG_EXPERIMENTAL and CVS_KEYWORD; see above.

- Josh Triplett

Re: [Ksummit-2013-discuss] [PATCH] checkpatch: Add comment about updating Documentation/CodingStyle

[Fengguang Wu]

On Mon, Sep 02, 2013 at 08:16:45PM -0700, Josh Triplett wrote:
> On Tue, Sep 03, 2013 at 10:46:40AM +0800, Fengguang Wu wrote:
> > On Mon, Sep 02, 2013 at 06:52:45PM -0700, Joe Perches wrote:
> > > On Mon, 2013-09-02 at 18:34 -0700, Josh Triplett wrote:
> > > > CONFIG_EXPERIMENTAL
> > > > CVS_KEYWORD
> > > 
> > > OK, but <shrug>
> [...]
> > Thanks for both of your suggestions! I'll add the commonly agreed ones:
> > 
> > +INVALID_UTF8
> > +LINUX_VERSION_CODE
> > +MISSING_EOF_NEWLINE
> > +HEXADECIMAL_BOOLEAN_TEST
> > +ALLOC_ARRAY_ARGS
> > +CONST_STRUCT
> > +CONSIDER_KSTRTO
> > 
> > And remove the duplicate one (good catch, Josh!)
> > 
> > -KREALLOC_ARG_REUSE
> 
> You missed CONFIG_EXPERIMENTAL and CVS_KEYWORD; see above.

OK, added, thanks!

Cheers,
Fengguang

Re: [Ksummit-2013-discuss] [PATCH] checkpatch: Add comment about updating Documentation/CodingStyle

[Bjorn Helgaas]

On Mon, Sep 2, 2013 at 6:39 PM, Fengguang Wu <fengguang.wu@intel.com> wrote:

> Thanks! Yes I'm now running checkpatch these days because some people
> suggested to me that some of the checkpatch warnings do help catch
> real bugs.
>
> However I do try to avoid upsetting people with maybe-subjective
> warnings. A checkpatch report will only be sent when a small fraction
> of error types are detected. Comments are very welcome on how to
> improve this list:

How hard would it be to make this configurable per-git tree?  I think
the robot is quite useful and I don't push branches very often, so I'd
probably be OK with just getting all the checkpatch warnings along
with the build warnings and filtering the noise myself.  But I know
other people have different styles and opinions.

> MEMSET
> IN_ATOMIC
> UAPI_INCLUDE
> MALFORMED_INCLUDE
> SIZEOF_ADDRESS
> KREALLOC_ARG_REUSE
> EXECUTE_PERMISSIONS
> ERROR:BAD_SIGN_OFF
> LO_MACRO
> HI_MACRO
> CSYNC
> SSYNC
> HOTPLUG_SECTION
> INDENTED_LABEL
> INLINE_LOCATION
> STORAGE_CLASS
> USLEEP_RANGE
> UNNECESSARY_CASTS
> ALLOC_SIZEOF_STRUCT
> KREALLOC_ARG_REUSE
> USE_FUNC
> LOCKDEP
> EXPORTED_WORLD_WRITABLE
> WHITESPACE_AFTER_LINE_CONTINUATION
> MISSING_VMLINUX_SYMBOL
> NEEDLESS_IF
> PRINTF_L
>
> Once the decision is made to send a checkpatch error/warning, the
> report email will use the triggering error (the one that matters) as
> the email subject, with the complete output of checkpatch.pl included
> in email body.
>
> Thanks,
> Fengguang
> _______________________________________________
> Ksummit-2013-discuss mailing list
> Ksummit-2013-discuss@lists.linuxfoundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/ksummit-2013-discuss

Re: [Ksummit-2013-discuss] [PATCH] checkpatch: Add comment about updating Documentation/CodingStyle

[Fengguang Wu]

On Tue, Sep 03, 2013 at 12:09:31PM -0600, Bjorn Helgaas wrote:
> On Mon, Sep 2, 2013 at 6:39 PM, Fengguang Wu <fengguang.wu@intel.com> wrote:
> 
> > Thanks! Yes I'm now running checkpatch these days because some people
> > suggested to me that some of the checkpatch warnings do help catch
> > real bugs.
> >
> > However I do try to avoid upsetting people with maybe-subjective
> > warnings. A checkpatch report will only be sent when a small fraction
> > of error types are detected. Comments are very welcome on how to
> > improve this list:
> 
> How hard would it be to make this configurable per-git tree?

It'd be trivial to do.

> I think the robot is quite useful and I don't push branches very
> often, so I'd probably be OK with just getting all the checkpatch
> warnings along with the build warnings and filtering the noise
> myself.  But I know other people have different styles and opinions.

Would you tell me the git tree/branches that would like to receive all
checkpatch warnings? It'll should be useful for our internal kernel
team, too.

Thanks,
Fengguang

> > MEMSET
> > IN_ATOMIC
> > UAPI_INCLUDE
> > MALFORMED_INCLUDE
> > SIZEOF_ADDRESS
> > KREALLOC_ARG_REUSE
> > EXECUTE_PERMISSIONS
> > ERROR:BAD_SIGN_OFF
> > LO_MACRO
> > HI_MACRO
> > CSYNC
> > SSYNC
> > HOTPLUG_SECTION
> > INDENTED_LABEL
> > INLINE_LOCATION
> > STORAGE_CLASS
> > USLEEP_RANGE
> > UNNECESSARY_CASTS
> > ALLOC_SIZEOF_STRUCT
> > KREALLOC_ARG_REUSE
> > USE_FUNC
> > LOCKDEP
> > EXPORTED_WORLD_WRITABLE
> > WHITESPACE_AFTER_LINE_CONTINUATION
> > MISSING_VMLINUX_SYMBOL
> > NEEDLESS_IF
> > PRINTF_L
> >
> > Once the decision is made to send a checkpatch error/warning, the
> > report email will use the triggering error (the one that matters) as
> > the email subject, with the complete output of checkpatch.pl included
> > in email body.
> >
> > Thanks,
> > Fengguang
> > _______________________________________________
> > Ksummit-2013-discuss mailing list
> > Ksummit-2013-discuss@lists.linuxfoundation.org
> > https://lists.linuxfoundation.org/mailman/listinfo/ksummit-2013-discuss

Re: [Ksummit-2013-discuss] [PATCH] checkpatch: Add comment about updating Documentation/CodingStyle

[Josh Triplett]

On Mon, Sep 02, 2013 at 09:50:23PM +0100, David Howells wrote:
> Josh Triplett <josh@joshtriplett.org> wrote:
> 
> > > There are many checkpatch rules (like semicolons) that
> > > are not in CodingStyle.
> > 
> > It's a rule of thumb, not a mandate.  In *general*, checkpatch.pl should
> > not be enforcing style rules that aren't documented in CodingStyle.
> 
> Except that it becomes a mandate when someone runs it automatically against
> every one of your patches and then sends you an email for each patch it finds
> a checkpatch niggle against...

I think we're talking about two different things.  I wasn't talking
about checkpatch.pl itself; I was talking about the idea that every
style rule in checkpatch.pl should corresponding documentation in
CodingStyle.  That's what I was calling a rule of thumb rather than a
mandate.

- Josh triplett

Re: [Ksummit-2013-discuss] [PATCH] checkpatch: Add comment about updating Documentation/CodingStyle

[Josh Triplett]

On Mon, Sep 02, 2013 at 03:39:45PM -0300, Mauro Carvalho Chehab wrote:
> Em Mon, 2 Sep 2013 11:19:01 -0700
> Josh Triplett <josh@joshtriplett.org> escreveu:
> 
> > Patches to checkpatch that add new style rules should also change
> > Documentation/CodingStyle to document those new style rules; add a
> > comment to that effect to the top of scripts/checkpatch.pl.
> 
> Well, you forgot to c/c LKML on this patch; I think that KS2013 is not the
> proper list to review this patch ;)

My mail went to LKML; it had:
To: linux-kernel@vger.kernel.org

> > Signed-off-by: Josh Triplett <josh@joshtriplett.org>
> > ---
> >  scripts/checkpatch.pl | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > index 2ee9eb7..ba65ea6 100755
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -4,6 +4,10 @@
> >  # (c) 2007,2008, Andy Whitcroft <apw@uk.ibm.com> (new conditions, test suite)
> >  # (c) 2008-2010 Andy Whitcroft <apw@canonical.com>
> >  # Licensed under the terms of the GNU GPL License version 2
> > +#
> > +# This file does not define the kernel coding style; Documentation/CodingStyle
> > +# does.  If you add a new style test to this file, add the corresponding style
> > +# rule it enforces to Documentation/CodingStyle.
> >  
> 
> Agreed with that. I would also add another comment there: "in case of
> conflicts between checkpatch.pl and Documentation/CodingStyle, the latter
> takes precedence."

Good point.

> Anyway,
> 
> Acked-by: Mauro Carvalho Chehab <m.chehab@samsung.com>

Thanks.

- Josh Triplett

[PATCH] checkpatch: Add warning about submitting patches using --file

[Joe Perches]

Add a message describing the lack of value in using
--file to generate patches.

Exclude files in staging from this message.

A similar message was removed by commit cf655043d4b
("update checkpatch.pl to version 0.15")

Signed-off-by: Joe Perches <joe@perches.com>
---

Maybe this sort of wordsmithing is valuable.

 scripts/checkpatch.pl | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 9bb056c..f788a14 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -4280,6 +4280,19 @@ $vname has style problems, please review.
 If any of these errors are false positives, please report
 them to the maintainer, see CHECKPATCH in MAINTAINERS.
 EOM
+		if ($file && $realfile !~ m@\bstaging/@) {
+			print << "EOM";
+
+WARNING: When using --file mode, do not send patches that just make
+whitespace or formatting changes unless more significant changes are
+also made for other reasons in another patch.
+
+Patches that are just code style changes have a real cost.
+
+These sort of patches can cause rejects with other changes and are
+thought of by many maintainers as more harmful and tiresome than useful.
+EOM
+		}
 	}
 
 	return $clean;

Re: [Ksummit-2013-discuss] [PATCH] checkpatch: Add warning about submitting patches using --file

[Mauro Carvalho Chehab]

Em Mon, 02 Sep 2013 12:40:47 -0700
Joe Perches <joe@perches.com> escreveu:

> Add a message describing the lack of value in using
> --file to generate patches.
> 
> Exclude files in staging from this message.
> 
> A similar message was removed by commit cf655043d4b
> ("update checkpatch.pl to version 0.15")
> 
> Signed-off-by: Joe Perches <joe@perches.com>

Acked-by: Mauro Carvalho Chehab <m.chehab@samsung.com>

> ---
> 
> Maybe this sort of wordsmithing is valuable.
> 
>  scripts/checkpatch.pl | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 9bb056c..f788a14 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -4280,6 +4280,19 @@ $vname has style problems, please review.
>  If any of these errors are false positives, please report
>  them to the maintainer, see CHECKPATCH in MAINTAINERS.
>  EOM
> +		if ($file && $realfile !~ m@\bstaging/@) {
> +			print << "EOM";
> +
> +WARNING: When using --file mode, do not send patches that just make
> +whitespace or formatting changes unless more significant changes are
> +also made for other reasons in another patch.
> +
> +Patches that are just code style changes have a real cost.
> +
> +These sort of patches can cause rejects with other changes and are
> +thought of by many maintainers as more harmful and tiresome than useful.
> +EOM
> +		}
>  	}
>  
>  	return $clean;
> 
> 
> _______________________________________________
> Ksummit-2013-discuss mailing list
> Ksummit-2013-discuss@lists.linuxfoundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/ksummit-2013-discuss


-- 

Cheers,
Mauro

Re: [PATCH] checkpatch: Add warning about submitting patches using --file

[Josh Triplett]

On Mon, Sep 02, 2013 at 12:40:47PM -0700, Joe Perches wrote:
> Add a message describing the lack of value in using
> --file to generate patches.
> 
> Exclude files in staging from this message.
> 
> A similar message was removed by commit cf655043d4b
> ("update checkpatch.pl to version 0.15")
> 
> Signed-off-by: Joe Perches <joe@perches.com>

Reviewed-by: Josh Triplett <josh@joshtriplett.org>

> ---
> 
> Maybe this sort of wordsmithing is valuable.
> 
>  scripts/checkpatch.pl | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 9bb056c..f788a14 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -4280,6 +4280,19 @@ $vname has style problems, please review.
>  If any of these errors are false positives, please report
>  them to the maintainer, see CHECKPATCH in MAINTAINERS.
>  EOM
> +		if ($file && $realfile !~ m@\bstaging/@) {
> +			print << "EOM";
> +
> +WARNING: When using --file mode, do not send patches that just make
> +whitespace or formatting changes unless more significant changes are
> +also made for other reasons in another patch.
> +
> +Patches that are just code style changes have a real cost.
> +
> +These sort of patches can cause rejects with other changes and are
> +thought of by many maintainers as more harmful and tiresome than useful.
> +EOM
> +		}
>  	}
>  
>  	return $clean;
> 
> 

Re: [PATCH] checkpatch: Add warning about submitting patches using --file

[Dan Carpenter]

On Mon, Sep 02, 2013 at 12:40:47PM -0700, Joe Perches wrote:
> +WARNING: When using --file mode, do not send patches that just make
> +whitespace or formatting changes unless more significant changes are
> +also made for other reasons in another patch.
> +

This is a run on sentence.  Also I don't agree with it.  Clean up
patches are good on their own.  There are parts of the kernel which are
not just in staging where I refuse to look at because it is so bad.

The problem is that people send "clean up" patches which don't clean up
the code or which make the code worse than the original.  All they care
about is pleasing checkpatch.pl instead of actually thinking about what
they are doing.  The message should just say something like, "Take a
step back and think about if this actually improves things for human
readers."

regards,
dan carpenter

Re: [PATCH] checkpatch: Add warning about submitting patches using --file

[Joe Perches]

On Mon, 2013-09-02 at 23:37 +0300, Dan Carpenter wrote:
> On Mon, Sep 02, 2013 at 12:40:47PM -0700, Joe Perches wrote:
> > +WARNING: When using --file mode, do not send patches that just make
> > +whitespace or formatting changes unless more significant changes are
> > +also made for other reasons in another patch.
> > +
> 
> This is a run on sentence.

Suggest alternatives.
I suppose "are also made" could be shortened.

> Also I don't agree with it.  Clean up
> patches are good on their own.

Try getting one past James "stasis" Bottomley.

> There are parts of the kernel which are
> not just in staging where I refuse to look at because it is so bad.

Me too.

> The problem is that people send "clean up" patches which don't clean up
> the code or which make the code worse than the original.

Maybe the only way to learn coding taste is to have
patches rejected.

> All they care
> about is pleasing checkpatch.pl instead of actually thinking about what
> they are doing.  The message should just say something like, "Take a
> step back and think about if this actually improves things for human
> readers."

Maybe.  Suggest better text.

Re: [PATCH] checkpatch: Add warning about submitting patches using --file

[Andrew Morton]

On Mon, 2 Sep 2013 23:37:05 +0300 Dan Carpenter <dan.carpenter@oracle.com> wrote:

> On Mon, Sep 02, 2013 at 12:40:47PM -0700, Joe Perches wrote:
> > +WARNING: When using --file mode, do not send patches that just make
> > +whitespace or formatting changes unless more significant changes are
> > +also made for other reasons in another patch.
> > +
> 
> This is a run on sentence.  Also I don't agree with it.  Clean up
> patches are good on their own.  There are parts of the kernel which are
> not just in staging where I refuse to look at because it is so bad.
> 
> The problem is that people send "clean up" patches which don't clean up
> the code or which make the code worse than the original.  All they care
> about is pleasing checkpatch.pl instead of actually thinking about what
> they are doing.  The message should just say something like, "Take a
> step back and think about if this actually improves things for human
> readers."

I don't agree either, really.  If someone sends a large cleanup patch
and it improves the code and comes at a suitable time, I'll happily
apply it, because it makes the kernel better.

Often these patches come from newbies and they've made various errors,
the most common of which is missed opportunities: there are cleanups
which should have been made but which weren't, due to timidity or to
lack of experience.  And that's OK - you point these things out, work
with the submitter and end up with a good patch and a happy and
better-informed submitter and a better kernel.  What's not to like
about that?

Sure, it takes time and it takes work.  But that's the maintainer's
problem, nobody else's.  Don't go assuming that your problems and
priorities are universal ones!