[PATCH] checkpatch: allow single space before labels

[PATCH] checkpatch: allow single space before labels

[Mike Frysinger]

Signed-off-by: Mike Frysinger <vapier@gentoo.org>
---
 scripts/checkpatch.pl |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 2039acd..1d06df7 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1459,7 +1459,7 @@ sub process {
 		}
 
 # check for spaces at the beginning of a line.
-		if ($rawline =~ /^\+ / && $rawline !~ /\+ +\*/)  {
+		if ($rawline =~ /^\+ / && $rawline !~ /\+ +\*/ && $rawline !~ /:$/)  {
 			my $herevet = "$here\n" . cat_vet($rawline) . "\n";
 			WARN("please, no space for starting a line, \
 				excluding comments\n" . $herevet);
-- 
1.7.3.1

Re: [PATCH] checkpatch: allow single space before labels

[Joe Perches]

On Sun, 2010-10-17 at 04:05 -0400, Mike Frysinger wrote:
> Signed-off-by: Mike Frysinger <vapier@gentoo.org>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
>  # check for spaces at the beginning of a line.
> -		if ($rawline =~ /^\+ / && $rawline !~ /\+ +\*/)  {
> +		if ($rawline =~ /^\+ / && $rawline !~ /\+ +\*/ && $rawline !~ /:$/)  {

Perhaps that's not an ideal test.
Maybe "&& $rawline !~ /^\+ \w+:/" is better.

The current test misidentify labels with comments:
drivers/net/tulip/interrupt.c: oom:    /* Executed with RX ints disabled */

Re: [PATCH] checkpatch: allow single space before labels

[Mike Frysinger]

On Sun, Oct 17, 2010 at 04:21, Joe Perches wrote:
> On Sun, 2010-10-17 at 04:05 -0400, Mike Frysinger wrote:
>> Signed-off-by: Mike Frysinger <vapier@gentoo.org>
>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> []
>>  # check for spaces at the beginning of a line.
>> -             if ($rawline =~ /^\+ / && $rawline !~ /\+ +\*/)  {
>> +             if ($rawline =~ /^\+ / && $rawline !~ /\+ +\*/ && $rawline !~ /:$/)  {
>
> Perhaps that's not an ideal test.
> Maybe "&& $rawline !~ /^\+ \w+:/" is better.
>
> The current test misidentify labels with comments:

i know, but i didnt want to dive further down this rabbit hole.  most
labels do not have trailing comments, or at least all the ones i deal
with.

feel free to extend however as long as the end result is no warnings
for " foo:" ;).
-mike

Re: [PATCH] checkpatch: allow single space before labels

[Peter Zijlstra]

On Sun, 2010-10-17 at 04:05 -0400, Mike Frysinger wrote:

NACK!

Use:

QUILT_DIFF_OPTS="-F ^[[:alpha:]\$_].*[^:]\$"


> Signed-off-by: Mike Frysinger <vapier@gentoo.org>
> ---
>  scripts/checkpatch.pl |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 2039acd..1d06df7 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -1459,7 +1459,7 @@ sub process {
>  		}
>  
>  # check for spaces at the beginning of a line.
> -		if ($rawline =~ /^\+ / && $rawline !~ /\+ +\*/)  {
> +		if ($rawline =~ /^\+ / && $rawline !~ /\+ +\*/ && $rawline !~ /:$/)  {
>  			my $herevet = "$here\n" . cat_vet($rawline) . "\n";
>  			WARN("please, no space for starting a line, \
>  				excluding comments\n" . $herevet);

Re: [PATCH] checkpatch: allow single space before labels

[Peter Zijlstra]

On Sun, 2010-10-17 at 04:24 -0400, Mike Frysinger wrote:

> feel free to extend however as long as the end result is no warnings
> for " foo:" ;).

I utterly detest those indented labels and am eradicating them wherever
I notice them. There's really no sane reason to use them what so ever.
Diff can be taught not to get confused about them, see my earlier email.

Re: [PATCH] checkpatch: allow single space before labels

[Mike Frysinger]

On Sun, Oct 17, 2010 at 06:21, Peter Zijlstra wrote:
> On Sun, 2010-10-17 at 04:24 -0400, Mike Frysinger wrote:
>> feel free to extend however as long as the end result is no warnings
>> for " foo:" ;).
>
> I utterly detest those indented labels and am eradicating them wherever
> I notice them. There's really no sane reason to use them what so ever.
> Diff can be taught not to get confused about them, see my earlier email.

sure, with enough shell code thrown at a problem, you can do anything.
 the change that started these warnings was for an unrelated check.
no one proposed warning on indented labels, nor is there any statement
at all in the coding style on these.

considering you highlighted the biggest reason (diff by default does
not handle them properly), i think that invalidates against your "no
sane reason" statement.
-mike

Re: [PATCH] checkpatch: allow single space before labels

[Peter Zijlstra]

On Sun, 2010-10-17 at 06:30 -0400, Mike Frysinger wrote:
> On Sun, Oct 17, 2010 at 06:21, Peter Zijlstra wrote:
> > On Sun, 2010-10-17 at 04:24 -0400, Mike Frysinger wrote:
> >> feel free to extend however as long as the end result is no warnings
> >> for " foo:" ;).
> >
> > I utterly detest those indented labels and am eradicating them wherever
> > I notice them. There's really no sane reason to use them what so ever.
> > Diff can be taught not to get confused about them, see my earlier email.
> 
> sure, with enough shell code thrown at a problem, you can do anything.
>  the change that started these warnings was for an unrelated check.
> no one proposed warning on indented labels, nor is there any statement
> at all in the coding style on these.
> 
> considering you highlighted the biggest reason (diff by default does
> not handle them properly), i think that invalidates against your "no
> sane reason" statement.

I think telling people to change their diff rules (--show-c-function
isn't default enabled either) is a lot better option than to uglify the
source. These indented labels are totally annoying when reading code.

Re: [PATCH] checkpatch: allow single space before labels

[Mike Frysinger]

On Sun, Oct 17, 2010 at 06:37, Peter Zijlstra wrote:
> On Sun, 2010-10-17 at 06:30 -0400, Mike Frysinger wrote:
>> On Sun, Oct 17, 2010 at 06:21, Peter Zijlstra wrote:
>> > On Sun, 2010-10-17 at 04:24 -0400, Mike Frysinger wrote:
>> >> feel free to extend however as long as the end result is no warnings
>> >> for " foo:" ;).
>> >
>> > I utterly detest those indented labels and am eradicating them wherever
>> > I notice them. There's really no sane reason to use them what so ever.
>> > Diff can be taught not to get confused about them, see my earlier email.
>>
>> sure, with enough shell code thrown at a problem, you can do anything.
>>  the change that started these warnings was for an unrelated check.
>> no one proposed warning on indented labels, nor is there any statement
>> at all in the coding style on these.
>>
>> considering you highlighted the biggest reason (diff by default does
>> not handle them properly), i think that invalidates against your "no
>> sane reason" statement.
>
> I think telling people to change their diff rules (--show-c-function
> isn't default enabled either) is a lot better option than to uglify the
> source. These indented labels are totally annoying when reading code.

and that is your opinion on the matter.  mine happens to go the
opposite way.  i prefer the single space when reading code.
-mike

Re: [PATCH] checkpatch: allow single space before labels

[Joe Perches]

On Sun, 2010-10-17 at 12:21 +0200, Peter Zijlstra wrote:
> On Sun, 2010-10-17 at 04:24 -0400, Mike Frysinger wrote:
> > feel free to extend however as long as the end result is no warnings
> > for " foo:" ;).
> I utterly detest those indented labels and am eradicating them wherever
> I notice them. There's really no sane reason to use them what so ever.
> Diff can be taught not to get confused about them, see my earlier email.

Your choice.

There are a half dozen or so in sched.c

There are 5000 or so single space indented labels.
There are 35000 or so labels without any indent.

His choice too no?

Re: [PATCH] checkpatch: allow single space before labels

[Peter Zijlstra]

On Sun, 2010-10-17 at 09:54 -0700, Joe Perches wrote:
> There are a half dozen or so in sched.c

Unlike some people, I've actually got something better to do than
whitespace patches.. they'll change next time the code in question needs
to change.

Re: [PATCH] checkpatch: allow single space before labels

[Joe Perches]

On Sun, 2010-10-17 at 21:30 +0200, Peter Zijlstra wrote:
> On Sun, 2010-10-17 at 09:54 -0700, Joe Perches wrote:
> > There are a half dozen or so in sched.c
> 
> Unlike some people, I've actually got something better to do than
> whitespace patches.. they'll change next time the code in question needs
> to change.

It takes as much time to reply as to change them.

perl -p -i -e 's/^ (\w+):/$1:/g' kernel/sched.c

Re: [PATCH] checkpatch: allow single space before labels

[Peter Zijlstra]

On Sun, 2010-10-17 at 12:38 -0700, Joe Perches wrote:
> On Sun, 2010-10-17 at 21:30 +0200, Peter Zijlstra wrote:
> > On Sun, 2010-10-17 at 09:54 -0700, Joe Perches wrote:
> > > There are a half dozen or so in sched.c
> > 
> > Unlike some people, I've actually got something better to do than
> > whitespace patches.. they'll change next time the code in question needs
> > to change.
> 
> It takes as much time to reply as to change them.
> 
> perl -p -i -e 's/^ (\w+):/$1:/g' kernel/sched.c

If you're skilled with such things as perl I guess, me I always forget
what uses which version of regexps and need to try longer to get the
regex right that it would take to change a few lines by hand.

Anyway, I applied it to kernel/sched*.c, thanks!

Re: [PATCH] checkpatch: allow single space before labels

[Mike Frysinger]

On Sun, Oct 17, 2010 at 15:30, Peter Zijlstra wrote:
> On Sun, 2010-10-17 at 09:54 -0700, Joe Perches wrote:
>> There are a half dozen or so in sched.c
>
> Unlike some people, I've actually got something better to do than
> whitespace patches.. they'll change next time the code in question needs
> to change.

sounds like a good reason to shut up the new unintended warnings
-mike

Re: [PATCH] checkpatch: allow single space before labels

[Peter Zijlstra]

On Sun, 2010-10-17 at 15:49 -0400, Mike Frysinger wrote:
> sounds like a good reason to shut up the new unintended warnings

I'm failing to see any logic there, old code doesn't generate warnings,
its new code that would, and new code should never have any whitespace
in front of labels.

Re: [PATCH] checkpatch: allow single space before labels

[Mike Frysinger]

On Sun, Oct 17, 2010 at 16:01, Peter Zijlstra wrote:
> On Sun, 2010-10-17 at 15:49 -0400, Mike Frysinger wrote:
>> sounds like a good reason to shut up the new unintended warnings
>
> I'm failing to see any logic there, old code doesn't generate warnings,
> its new code that would, and new code should never have any whitespace
> in front of labels.

it's a waste of time to massage code to one person's opinion
-mike

Re: [PATCH] checkpatch: allow single space before labels

[Ted Ts'o]

On Sun, Oct 17, 2010 at 04:26:25PM -0400, Mike Frysinger wrote:
> On Sun, Oct 17, 2010 at 16:01, Peter Zijlstra wrote:
> > On Sun, 2010-10-17 at 15:49 -0400, Mike Frysinger wrote:
> >> sounds like a good reason to shut up the new unintended warnings
> >
> > I'm failing to see any logic there, old code doesn't generate warnings,
> > its new code that would, and new code should never have any whitespace
> > in front of labels.
> 
> it's a waste of time to massage code to one person's opinion

+1

It's not in CondingStyle; I prefer to outdent labels by one tab stop:

			if (!page_has_buffers(page)) {
				if (block_prepare_write(page, 0, len,
						noalloc_get_block_write)) {
				redirty_page:
					redirty_page_for_writepage(mpd->wbc,
								   page);
					unlock_page(page);
					continue;
				}
				commit_write = 1;
			}

... and I hereby serve notice that if I start getting trash patches
from newbies about checkpatch.pl warning/errors for stuff like this,
I'll will (a) toast them and tell them they are wasting their time
with checkpatch.pl, and (b) agitate strongly for the removal of such
trash checkpatch.pl or for checkpatch.pl as a whole, as being harmful
to kernel development.

						- Ted

Re: [PATCH] checkpatch: allow single space before labels

[Ted Ts'o]

On Mon, Oct 18, 2010 at 10:39:35AM -0400, Ted Ts'o wrote:
> On Sun, Oct 17, 2010 at 04:26:25PM -0400, Mike Frysinger wrote:
> > On Sun, Oct 17, 2010 at 16:01, Peter Zijlstra wrote:
> > > On Sun, 2010-10-17 at 15:49 -0400, Mike Frysinger wrote:
> > >> sounds like a good reason to shut up the new unintended warnings
> > >
> > > I'm failing to see any logic there, old code doesn't generate warnings,
> > > its new code that would, and new code should never have any whitespace
> > > in front of labels.
> > 
> > it's a waste of time to massage code to one person's opinion
> 
> +1
> 
> It's not in CondingStyle; I prefer to outdent labels by one tab stop:

Note that there is absolutely nothing about how labels should be
indented in CodingStyle at all, and I very much resist code
straightjackets being imposed by checkpatch.pl implementors.

Can we please stop this nonsense?

						- Ted

Re: [PATCH] checkpatch: allow single space before labels

[Peter Zijlstra]

On Mon, 2010-10-18 at 10:54 -0400, Ted Ts'o wrote:
> Note that there is absolutely nothing about how labels should be
> indented in CodingStyle at all, and I very much resist code
> straightjackets being imposed by checkpatch.pl implementors.
> 
> Can we please stop this nonsense? 

1) ignoring checkpatch over taste is good
2) by 1) its impossible to get checkpatch right
3) therefore pure checkpatch patches are nonsense

So can we agree that checkpatch is ok in its current form, and we should
simply administer physical violence towards checkpatch wankers?

PS. I frequently practise 2).
PPS. I find your preferred label indenting truly hideous.

Re: [PATCH] checkpatch: allow single space before labels

[Peter Zijlstra]

On Mon, 2010-10-18 at 16:58 +0200, Peter Zijlstra wrote:
> On Mon, 2010-10-18 at 10:54 -0400, Ted Ts'o wrote:
> > Note that there is absolutely nothing about how labels should be
> > indented in CodingStyle at all, and I very much resist code
> > straightjackets being imposed by checkpatch.pl implementors.
> > 
> > Can we please stop this nonsense? 
> 
> 1) ignoring checkpatch over taste is good
> 2) by 1) its impossible to get checkpatch right
> 3) therefore pure checkpatch patches are nonsense
> 
> So can we agree that checkpatch is ok in its current form, and we should
> simply administer physical violence towards checkpatch wankers?
> 
> PS. I frequently practise 2).

That should be 1) of course.. I frequently ignore checkpatch output.

Re: [PATCH] checkpatch: allow single space before labels

[Mike Frysinger]

On Mon, Oct 18, 2010 at 10:58, Peter Zijlstra wrote:
> On Mon, 2010-10-18 at 10:54 -0400, Ted Ts'o wrote:
>> Note that there is absolutely nothing about how labels should be
>> indented in CodingStyle at all, and I very much resist code
>> straightjackets being imposed by checkpatch.pl implementors.
>>
>> Can we please stop this nonsense?
>
> 1) ignoring checkpatch over taste is good
> 2) by 1) its impossible to get checkpatch right
> 3) therefore pure checkpatch patches are nonsense

sounds like an ACK for Joe's patch

Joe: could you post an updated patch ?
-mke

Re: [PATCH] checkpatch: allow single space before labels

[Peter Zijlstra]

On Mon, 2010-10-18 at 14:21 -0400, Mike Frysinger wrote:
> On Mon, Oct 18, 2010 at 10:58, Peter Zijlstra wrote:
> > On Mon, 2010-10-18 at 10:54 -0400, Ted Ts'o wrote:
> >> Note that there is absolutely nothing about how labels should be
> >> indented in CodingStyle at all, and I very much resist code
> >> straightjackets being imposed by checkpatch.pl implementors.
> >>
> >> Can we please stop this nonsense?
> >
> > 1) ignoring checkpatch over taste is good
> > 2) by 1) its impossible to get checkpatch right
> > 3) therefore pure checkpatch patches are nonsense
> 
> sounds like an ACK for Joe's patch

No its quite the reverse, by acking joe's patch I loose checkpatch
functionality I like.

There are checks I don't really like, for those I simply ignore its
output, I suggest others do the same.

Re: [PATCH] checkpatch: allow single space before labels

[Joe Perches]

On Mon, 2010-10-18 at 20:32 +0200, Peter Zijlstra wrote:
> On Mon, 2010-10-18 at 14:21 -0400, Mike Frysinger wrote:
> > sounds like an ACK for Joe's patch

I didn't submit a patch, I suggested a possible improvement to
your patch.  If you agree with it, you could resubmit.

> There are checks I don't really like, for those I simply ignore its
> output, I suggest others do the same.

Seems sensible.

Mike, checkpatch does already allow a single space to precede
a label but there's a conflict between the leading spaces test
and this test below it, so something like your patch is useful.

----------------
#goto labels aren't indented, allow a single space however
		if ($line=~/^.\s+[A-Za-z\d_]+:(?![0-9]+)/ and
		   !($line=~/^. [A-Za-z\d_]+:/) and !($line=~/^.\s+default:/)) {
			WARN("labels should not be indented\n" . $herecurr);
		}
----------------

The leading space test may be overly noisy.
Maybe Andy can fix it a different way.

About 1% (~500) of the labels in the kernel source tree use
leading tabs.  I think that label style is harder for me to
visually scan and find, but it's probably not code I'd change
just for conformity.