Checkpatch Feature Idea: Search directory for files with errors and warnings with -d argument

Checkpatch Feature Idea: Search directory for files with errors and warnings with -d argument

[Nick Krause]

I may have not found it myself but if it doesn't exist can we write a
feature for checkpatch to be able to recursively
search a directory structure with a -d argument in order to make it
easier to search larger directories for files that still
need cleanup for files having kernel coding style issues. Furthermore
I am asking you guys to write this feature
if you feel this is a good idea as I am only a beginner in perl. I can
help out by testing  it however if needed :).
Cheers Nick

Re: Checkpatch Feature Idea: Search directory for files with errors and warnings with -d argument

[Joe Perches]

On Tue, 2014-07-15 at 22:50 -0400, Nick Krause wrote:
> I may have not found it myself but if it doesn't exist can we write a
> feature for checkpatch to be able to recursively
> search a directory structure with a -d argument in order to make it
> easier to search larger directories for files that still
> need cleanup for files having kernel coding style issues.

xargs already works fine.

git ls-files <dir>/*.[ch] | xargs ./scripts/checkpatch.pl -f

I suggest you only use anything like this
on staging directories.

Re: Checkpatch Feature Idea: Search directory for files with errors and warnings with -d argument

[Nick Krause]

On Tue, Jul 15, 2014 at 11:38 PM, Joe Perches <joe@perches.com> wrote:
> On Tue, 2014-07-15 at 22:50 -0400, Nick Krause wrote:
>> I may have not found it myself but if it doesn't exist can we write a
>> feature for checkpatch to be able to recursively
>> search a directory structure with a -d argument in order to make it
>> easier to search larger directories for files that still
>> need cleanup for files having kernel coding style issues.
>linux-kernel@vger.kernel.org
> xargs already works fine.
>
> git ls-files <dir>/*.[ch] | xargs ./scripts/checkpatch.pl -f
>
> I suggest you only use anything like this
> on staging directories.
>
>

Thanks Joe,
That was what I needed to known :). I was hitting some errors in arch,
so I will run that to see if there are any others.
Cheers Nick

Re: Checkpatch Feature Idea: Search directory for files with errors and warnings with -d argument

[Joe Perches]

On Wed, 2014-07-16 at 00:16 -0400, Nick Krause wrote:
> On Tue, Jul 15, 2014 at 11:38 PM, Joe Perches <joe@perches.com> wrote:
> > On Tue, 2014-07-15 at 22:50 -0400, Nick Krause wrote:
> >> I may have not found it myself but if it doesn't exist can we write a
> >> feature for checkpatch to be able to recursively
> >> search a directory structure with a -d aRrgument in order to make it
> >> easier to search larger directories for files that still
> >> need cleanup for files having kernel coding style issues.
> >linux-kernel@vger.kernel.org
> > xargs already works fine.
> >
> > git ls-files <dir>/*.[ch] | xargs ./scripts/checkpatch.pl -f
> >
> > I suggest you only use anything like this
> > on staging directories.
> >
> >
> 
> Thanks Joe,
> That was what I needed to known :). I was hitting some errors in arch,
> so I will run that to see if there are any others.
> Cheers Nick

I again suggest you _ONLY_ use checkpatch on staging.

Doing checkpatch only cleanups will not make you a
better developer nor give you a good understanding
of the code operation.

Re: Checkpatch Feature Idea: Search directory for files with errors and warnings with -d argument

[Nick Krause]

On Wed, Jul 16, 2014 at 12:23 AM, Joe Perches <joe@perches.com> wrote:
> On Wed, 2014-07-16 at 00:16 -0400, Nick Krause wrote:
>> On Tue, Jul 15, 2014 at 11:38 PM, Joe Perches <joe@perches.com> wrote:
>> > On Tue, 2014-07-15 at 22:50 -0400, Nick Krause wrote:
>> >> I may have not found it myself but if it doesn't exist can we write a
>> >> feature for checkpatch to be able to recursively
>> >> search a directory structure with a -d aRrgument in order to make it
>> >> easier to search larger directories for files that still
>> >> need cleanup for files having kernel coding style issues.
>> >linux-kernel@vger.kernel.org
>> > xargs already works fine.
>> >
>> > git ls-files <dir>/*.[ch] | xargs ./scripts/checkpatch.pl -f
>> >
>> > I suggest you only use anything like this
>> > on staging directories.
>> >
>> >
>>
>> Thanks Joe,
>> That was what I needed to known :). I was hitting some errors in arch,
>> so I will run that to see if there are any others.
>> Cheers Nick
>
> I again suggest you _ONLY_ use checkpatch on staging.
>
> Doing checkpatch only cleanups will not make you a
> better developer nor give you a good understanding
> of the code operation.
>

I am cleaning up the kernel as it needs a lot of cleanup.  In addition
I am doing build related issues.
Some developers removed me from FIX MES after several stupid patches I
sent. If there is a area
that needs help , please let me know :).

Re: Checkpatch Feature Idea: Search directory for files with errors and warnings with -d argument

[Joe Perches]

On Wed, 2014-07-16 at 00:28 -0400, Nick Krause wrote:
> I am cleaning up the kernel as it needs a lot of cleanup.

Needs are curious things.

Consistency is a nicety not really a need.

Bugs need fixing.  Defects need eliminating.
Enhancements are appreciated.  Inconsistent
code style is a minor annoyance.

I suggest you focus on the bugs, defects or
enhancements in performance or testing.

Are you really cross-compiling the patches you
submit here or do you have an alpha on your desk?

Re: Checkpatch Feature Idea: Search directory for files with errors and warnings with -d argument

[Nick Krause]

On Wed, Jul 16, 2014 at 12:39 AM, Joe Perches <joe@perches.com> wrote:
> On Wed, 2014-07-16 at 00:28 -0400, Nick Krause wrote:
>> I am cleaning up the kernel as it needs a lot of cleanup.
>
> Needs are curious things.
>
> Consistency is a nicety not really a need.
>
> Bugs need fixing.  Defects need eliminating.
> Enhancements are appreciated.  Inconsistent
> code style is a minor annoyance.
>
> I suggest you focus on the bugs, defects or
> enhancements in performance or testing.
>
> Are you really cross-compiling the patches you
> submit here or do you have an alpha on your desk?
>
>
I am cross compiling then. I don't have the hardware :(.
Nick

Re: Checkpatch Feature Idea: Search directory for files with errors and warnings with -d argument

[Sasha Levin]

On 07/16/2014 12:59 AM, Nick Krause wrote:
> On Wed, Jul 16, 2014 at 12:39 AM, Joe Perches <joe@perches.com> wrote:
>> > On Wed, 2014-07-16 at 00:28 -0400, Nick Krause wrote:
>>> >> I am cleaning up the kernel as it needs a lot of cleanup.
>> >
>> > Needs are curious things.
>> >
>> > Consistency is a nicety not really a need.
>> >
>> > Bugs need fixing.  Defects need eliminating.
>> > Enhancements are appreciated.  Inconsistent
>> > code style is a minor annoyance.
>> >
>> > I suggest you focus on the bugs, defects or
>> > enhancements in performance or testing.
>> >
>> > Are you really cross-compiling the patches you
>> > submit here or do you have an alpha on your desk?
>> >
>> >
> I am cross compiling then. I don't have the hardware :(.

Are you sure you're actually cross compiling them? From your
recent patchset it doesn't seem you even bother to do that:

diff --git a/arch/alpha/boot/misc.c b/arch/alpha/boot/misc.c
index 174b7c6..886e469 100644
--- a/arch/alpha/boot/misc.c
+++ b/arch/alpha/boot/misc.c
@@ -23,7 +23,7 @@

 #include <asm/uaccess.h>

-#define memzero (s, n)	memset((s), 0 , (n))
+#define memzero { (s, n)	memset((s), 0 , (n)) }
 #define puts		srm_printk
 extern long srm_printk(const char *, ...)
 	__attribute__ ((format (printf, 1, 2)));
@@ -61,8 +61,8 @@ static unsigned outcnt;		/* bytes in output buffer */

 /* Diagnostic functions */
 #ifdef DEBUG
-#  define Assert (cond, msg) {if (!(cond)) error(msg) ; }
-#  define Trace(x) fprintf x
+#  define Assert { (cond, msg) {if (!(cond)) error(msg) ; } }
+#  define Trace { (x) fprintf x }
 #  define Tracev(x) {if (verbose) fprintf x ; }
 #  define Tracevv(x) {if (verbose > 1) fprintf x ; }
 #  define Tracec(c, x) {if (verbose && (c)) fprintf x ; }

Or:

diff --git a/arch/alpha/boot/misc.c b/arch/alpha/boot/misc.c
index 886e469..56c3325 100644
--- a/arch/alpha/boot/misc.c
+++ b/arch/alpha/boot/misc.c
@@ -144,7 +144,7 @@ static void error(char *x)
 	puts(x);
 	puts("\n\n -- System halted");

-	while (1);
+	while{ (1); }
 	/* Halt */
 }


Thanks,
Sasha

Re: Checkpatch Feature Idea: Search directory for files with errors and warnings with -d argument

[Theodore Ts'o]

On Tue, Jul 15, 2014 at 09:39:52PM -0700, Joe Perches wrote:
> On Wed, 2014-07-16 at 00:28 -0400, Nick Krause wrote:
> > I am cleaning up the kernel as it needs a lot of cleanup.
> 
> Needs are curious things.
> 
> Consistency is a nicety not really a need.
> 
> Bugs need fixing.  Defects need eliminating.
> Enhancements are appreciated.  Inconsistent
> code style is a minor annoyance.

Note that patches that clean up code style can in fact be actively
harmful, because it interferes with other developers who are sending
patches that fix real bugs and and add new features (because the
cleanups cause patch conflicts).

In general I will ignore patches from people who run checkpatch on
files thinking they are doing me a feature by "cleaning up" the code.
Cleanups as a part of normal code development is fine.

Note also that checkpatch in the past has been at fault for a huge
amount of code churn.  At one point, checkpatch would whine whever
lines were longer than 80 characters, causing people to send huge
numbers of pointles spatch to split printk strings across multiple
lines.  Now, checkpatch whines when it sees character strings that
span multiple lines; so other people will send equally pointless
patches to rejoin those character strings.

In general, checkpatch is good for fixing up patches so that the
maintainer won't complain over stupid nit-picky things.  But fixing
every single nit that checkpatch whines about in a while adds no
value, and in fact, can add negative value.

(Also note that there are some local coding practices where there are
very good reasons why the checkpatch whines need to be completely
ignored.  For example, you checkpatch doesn't deal well with the file
format used in include/tracing/events/*.h.  You need to know when the
right thing to do is to say, "Go home, checkpatch, you're drunk.")

						- Ted

Re: Checkpatch Feature Idea: Search directory for files with errors and warnings with -d argument

[Joe Perches]

On Wed, 2014-07-16 at 04:05 -0400, Theodore Ts'o wrote:
> On Tue, Jul 15, 2014 at 09:39:52PM -0700, Joe Perches wrote:
[]
> > Consistency is a nicety not really a need.
> > 
> > Bugs need fixing.  Defects need eliminating.
> > Enhancements are appreciated.  Inconsistent
> > code style is a minor annoyance.
> 
> Note that patches that clean up code style can in fact be actively
> harmful, because it interferes with other developers who are sending
> patches that fix real bugs and and add new features (because the
> cleanups cause patch conflicts).
[]
> Cleanups as a part of normal code development is fine.
[]
> fixing
> every single nit that checkpatch whines about in a while adds no
> value, and in fact, can add negative value.

I think the negatives are a bit overstated.

As long as style only patches are scheduled, coordinated,
and done with a much lower priority than other real work,
there is some marginal positive value in code style
consistency.

Humans more easily read code of multiple subsystems.
Defects in existing code are sometimes uncovered.
More consistent API use is encouraged.

There are real tradeoffs to be considered in developer
time vs value though.

Even whitespace changes need review and acceptance from
upstream maintainers to make sure defects and new security
issues aren't introduced.

Upstream maintainer time is not cheap and frequently the
upstream maintainer is not cheerful either.

> (Also note that there are some local coding practices where there are
> very good reasons why the checkpatch whines need to be completely
> ignored.  For example, you checkpatch doesn't deal well with the file
> format used in include/tracing/events/*.h.  You need to know when the
> right thing to do is to say, "Go home, checkpatch, you're drunk.")

checkpatch can be a bit of an OCD drunk.  Codetenders
have the responsibility to cut it off when its spent
too long at the file bar.

Of course it's true that checkpatch needs to have its
own bugs fixed and defects eliminated too.