(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
>
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
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 <[email protected]>
---
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 <[email protected]> (new conditions, test suite)
# (c) 2008-2010 Andy Whitcroft <[email protected]>
# 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
Em Mon, 2 Sep 2013 11:19:01 -0700
Josh Triplett <[email protected]> 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 <[email protected]>
> ---
> 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 <[email protected]> (new conditions, test suite)
> # (c) 2008-2010 Andy Whitcroft <[email protected]>
> # 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 <[email protected]>
> use strict;
> use POSIX;
Regard
--
Cheers,
Mauro
On Mon, 2013-09-02 at 15:39 -0300, Mauro Carvalho Chehab wrote:
> Em Mon, 2 Sep 2013 11:19:01 -0700
> Josh Triplett <[email protected]> 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.
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 <[email protected]> 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: [email protected]
> > Signed-off-by: Josh Triplett <[email protected]>
> > ---
> > 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 <[email protected]> (new conditions, test suite)
> > # (c) 2008-2010 Andy Whitcroft <[email protected]>
> > # 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 <[email protected]>
Thanks.
- Josh Triplett
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 <[email protected]>
---
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;
Em Mon, 02 Sep 2013 11:59:27 -0700
Joe Perches <[email protected]> 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 <[email protected]> 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
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 <[email protected]> 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
Em Mon, 02 Sep 2013 12:40:47 -0700
Joe Perches <[email protected]> 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 <[email protected]>
Acked-by: Mauro Carvalho Chehab <[email protected]>
> ---
>
> 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
> [email protected]
> https://lists.linuxfoundation.org/mailman/listinfo/ksummit-2013-discuss
--
Cheers,
Mauro
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 <[email protected]>
Reviewed-by: Josh Triplett <[email protected]>
> ---
>
> 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;
>
>
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 <[email protected]> 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.
>
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
Josh Triplett <[email protected]> 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
On Mon, 2013-09-02 at 21:50 +0100, David Howells wrote:
> Josh Triplett <[email protected]> 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 <[email protected]>
sent me an automated checkpatch email I
thought was not useful.
Does anyone know of other checkpatch robots?
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.
On Mon, Sep 02, 2013 at 09:50:23PM +0100, David Howells wrote:
> Josh Triplett <[email protected]> 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
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 <[email protected]>
---
> 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+//;
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 <[email protected]>
Reviewed-by: Josh Triplett <[email protected]>
> --- 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
> 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]) . " ";
2013/9/3 Joe Perches <[email protected]>:
> On Mon, 2013-09-02 at 21:50 +0100, David Howells wrote:
>> Josh Triplett <[email protected]> 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 <[email protected]>
> 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?
>
>
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 <[email protected]>
On Tue, Sep 03, 2013 at 08:26:21AM +0800, Shilong Wang wrote:
> 2013/9/3 Joe Perches <[email protected]>:
> > Wang Shilong <[email protected]>
> > 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
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 <[email protected]> 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
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
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 <[email protected]>:
> > > Wang Shilong <[email protected]>
> > > 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 @[email protected]>'
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
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 <[email protected]> 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
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
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>
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
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
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
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
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
On Mon, Sep 2, 2013 at 6:39 PM, Fengguang Wu <[email protected]> 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
> [email protected]
> https://lists.linuxfoundation.org/mailman/listinfo/ksummit-2013-discuss
On Tue, Sep 03, 2013 at 12:09:31PM -0600, Bjorn Helgaas wrote:
> On Mon, Sep 2, 2013 at 6:39 PM, Fengguang Wu <[email protected]> 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
> > [email protected]
> > https://lists.linuxfoundation.org/mailman/listinfo/ksummit-2013-discuss
On Mon, 2 Sep 2013 23:37:05 +0300 Dan Carpenter <[email protected]> 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!