All of lore.kernel.org
 help / color / mirror / Atom feed
From: Joe Perches <joe@perches.com>
To: Graeme Russ <graeme.russ@gmail.com>,
	Andy Whitcroft <apw@canonical.com>,
	Andrew Morton <akpm@linux-foundation.org>
Cc: LKML <linux-kernel@vger.kernel.org>, U-Boot Users <u-boot@lists.denx.de>
Subject: Re: Expanding checkpatch for non-linux (specifically U-Boot) use
Date: Sun, 24 Apr 2011 23:02:20 -0700	[thread overview]
Message-ID: <1303711340.1745.30.camel@Joe-Laptop> (raw)
In-Reply-To: <4DB50854.8090700@gmail.com>

On Mon, 2011-04-25 at 15:36 +1000, Graeme Russ wrote:
> There has been a bit of discussion lately on the U-Boot mailing list
> regarding the use of checkpatch for U-Boot patches (see
> http://lists.denx.de/pipermail/u-boot/2011-April/090954.html)
> 
> U-Boot uses the Linux coding style and checkpatch is therefore a very good
> tool for us to use to check style compliance. However, checkpatch has a few
> Linux specific checks which throw up false warnings for U-Boot patches like:
>
> WARNING: consider using kstrto* in preference to simple_strto*
> WARNING: Use #include <linux/$file> instead of <asm/$file>
> 
> Also, checkpatch seems to be checking not only patched lines, but context
> lines as well. There is a policy for U-Boot patches to not intermix
> whitespace / code cleanup changes and functional changes in in the same
> patch. So to achieve zero warnings and errors, the submitter is forced to
> create an additional code-cleanup patch in addition to the functionality
> patch. The code cleanup can end up being significantly larger than the
> functionality change which discourages casual submitters.
> 
> So I have a pretty simple question to ask of LKML - Will checkpatch patches
> to create a 'U-Boot' command-line option to explicitly filter out Linux
> specific warnings and errors ever be accepted into checkpatch, or will we
> be required to create and maintain a U-Boot specific version?
> 
> P.S. If you could please keep the U-Boot mailing list Cc'd, that would be
> appreciated

Hi Graeme.

Perhaps some sort of .checkpatch.conf file
could be introduced which could be linked to
specific types of errors/warnings/checks
that should be reported or ignored.

checkpatch has central routines to emit messages.

sub ERROR {
	if (report("ERROR: $_[0]\n")) {
		our $clean = 0;
		our $cnt_error++;
	}
}
sub WARN {
	if (report("WARNING: $_[0]\n")) {
		our $clean = 0;
		our $cnt_warn++;
	}
}
sub CHK {
	if ($check && report("CHECK: $_[0]\n")) {
		our $clean = 0;
		our $cnt_chk++;
	}
}

For instance, warnings could be changed to include
a new unique identifier for each message.

from
	WARN("Signed-off-by: is the preferred form\n" .
					$herecurr);
to
	WARN($WARN_SIGN_OFF,
	     "Signed-off-by: is the preferred form\n" .
					$herecurr);

and the ERROR/WARN/CHK routines could be extended to use
entries in the .conf file to enable/disable each message.

uboot could then use an appropriate .conf file.



WARNING: multiple messages have this Message-ID (diff)
From: Joe Perches <joe@perches.com>
To: u-boot@lists.denx.de
Subject: [U-Boot] Expanding checkpatch for non-linux (specifically U-Boot) use
Date: Sun, 24 Apr 2011 23:02:20 -0700	[thread overview]
Message-ID: <1303711340.1745.30.camel@Joe-Laptop> (raw)
In-Reply-To: <4DB50854.8090700@gmail.com>

On Mon, 2011-04-25 at 15:36 +1000, Graeme Russ wrote:
> There has been a bit of discussion lately on the U-Boot mailing list
> regarding the use of checkpatch for U-Boot patches (see
> http://lists.denx.de/pipermail/u-boot/2011-April/090954.html)
> 
> U-Boot uses the Linux coding style and checkpatch is therefore a very good
> tool for us to use to check style compliance. However, checkpatch has a few
> Linux specific checks which throw up false warnings for U-Boot patches like:
>
> WARNING: consider using kstrto* in preference to simple_strto*
> WARNING: Use #include <linux/$file> instead of <asm/$file>
> 
> Also, checkpatch seems to be checking not only patched lines, but context
> lines as well. There is a policy for U-Boot patches to not intermix
> whitespace / code cleanup changes and functional changes in in the same
> patch. So to achieve zero warnings and errors, the submitter is forced to
> create an additional code-cleanup patch in addition to the functionality
> patch. The code cleanup can end up being significantly larger than the
> functionality change which discourages casual submitters.
> 
> So I have a pretty simple question to ask of LKML - Will checkpatch patches
> to create a 'U-Boot' command-line option to explicitly filter out Linux
> specific warnings and errors ever be accepted into checkpatch, or will we
> be required to create and maintain a U-Boot specific version?
> 
> P.S. If you could please keep the U-Boot mailing list Cc'd, that would be
> appreciated

Hi Graeme.

Perhaps some sort of .checkpatch.conf file
could be introduced which could be linked to
specific types of errors/warnings/checks
that should be reported or ignored.

checkpatch has central routines to emit messages.

sub ERROR {
	if (report("ERROR: $_[0]\n")) {
		our $clean = 0;
		our $cnt_error++;
	}
}
sub WARN {
	if (report("WARNING: $_[0]\n")) {
		our $clean = 0;
		our $cnt_warn++;
	}
}
sub CHK {
	if ($check && report("CHECK: $_[0]\n")) {
		our $clean = 0;
		our $cnt_chk++;
	}
}

For instance, warnings could be changed to include
a new unique identifier for each message.

from
	WARN("Signed-off-by: is the preferred form\n" .
					$herecurr);
to
	WARN($WARN_SIGN_OFF,
	     "Signed-off-by: is the preferred form\n" .
					$herecurr);

and the ERROR/WARN/CHK routines could be extended to use
entries in the .conf file to enable/disable each message.

uboot could then use an appropriate .conf file.

  reply	other threads:[~2011-04-25  6:02 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-04-25  5:36 Expanding checkpatch for non-linux (specifically U-Boot) use Graeme Russ
2011-04-25  5:36 ` [U-Boot] " Graeme Russ
2011-04-25  6:02 ` Joe Perches [this message]
2011-04-25  6:02   ` Joe Perches
2011-04-25  6:22   ` Graeme Russ
2011-04-25  6:22     ` [U-Boot] " Graeme Russ
2011-04-25  6:32     ` Joe Perches
2011-04-25  6:32       ` [U-Boot] " Joe Perches
2011-04-26  0:43       ` [RFC PATCH] checkpatch.pl: Add ability to ignore various messages Joe Perches
2011-04-26  0:43         ` [U-Boot] " Joe Perches
2011-06-09 19:26         ` Joe Perches
2011-06-09 19:26           ` [U-Boot] " Joe Perches
2011-06-09 19:40           ` Mike Frysinger
2011-06-09 19:40             ` Mike Frysinger
2011-07-27 21:17         ` Wolfgang Denk
2011-07-27 21:17           ` Wolfgang Denk
2011-07-27 22:10           ` Joe Perches
2011-07-27 22:10             ` Joe Perches
2011-08-24 21:25             ` Wolfgang Denk
2011-08-24 21:25               ` Wolfgang Denk
2011-08-24 21:33               ` Joe Perches
2011-08-24 21:33                 ` Joe Perches

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1303711340.1745.30.camel@Joe-Laptop \
    --to=joe@perches.com \
    --cc=akpm@linux-foundation.org \
    --cc=apw@canonical.com \
    --cc=graeme.russ@gmail.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=u-boot@lists.denx.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.