[patch] checkpatch: relax spacing and line length

[patch] checkpatch: relax spacing and line length

[Jan Engelhardt]

===
total: 0 errors, 0 warnings, 36 lines checked

d27a9f5.diff has no obvious style problems and is ready for submission.
===
commit d27a9f5760fa231ab888f96e27355a001c88b239
Author: Jan Engelhardt <jengelh@computergmbh.de>
Date:   Sun Apr 6 06:49:01 2008 +0200

     checkpatch: relax spacing and line length

     We all had the arguments about 80 columns, so here goes a relax.
     Checking for 95 (or perhaps something better?), but of course we
     print "80" in the output, because if you happened to get to 95, it's
     "really time" to break it.

     This also relaxes the tab doctrine, because spaces DO make sense --
     especially when you view the code with a tab setting of not-8.

     Signed-off-by: Jan Engelhardt <jengelh@computergmbh.de>
---
  scripts/checkpatch.pl |   22 ++++++++++++++++------
  1 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 58a9494..e5a96c1 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1094,8 +1094,8 @@ sub process {
  			my $herevet = "$here\n" . cat_vet($rawline) . "\n";
  			ERROR("trailing whitespace\n" . $herevet);
  		}
-#80 column limit
-		if ($line =~ /^\+/ && !($prevrawline=~/\/\*\*/) && $length > 80) {
+#80 column limit (yes, testing for 95 is correct, we do not want to annoy)
+		if ($line =~ /^\+/ && !($prevrawline=~/\/\*\*/) && $length >= 95) {
  			WARN("line over 80 characters\n" . $herecurr);
  		}

@@ -1107,12 +1107,22 @@ sub process {
  # check we are in a valid source file *.[hc] if not then ignore this hunk
  		next if ($realfile !~ /\.[hc]$/);

+		my $arg = qr/$Type(?: ?$Ident)?/;
+		if ($rawline =~ /^\+ +($arg, )*$arg(?:,|\);?)$/) {
+			# We are probably in a function parameter list.
+			# Spaces ok -- used for align.
+		} elsif ($rawline =~ /^\+\t+ *\S/) {
+			# We are probably in a function or an array/struct
+			# definition/initializer. Spaces ok -- used for align
+			# on multiline statements.
+		} else {
  # at the beginning of a line any tabs must come first and anything
  # more than 8 must use tabs.
-		if ($rawline =~ /^\+\s* \t\s*\S/ ||
-		    $rawline =~ /^\+\s*        \s*/) {
-			my $herevet = "$here\n" . cat_vet($rawline) . "\n";
-			ERROR("use tabs not spaces\n" . $herevet);
+			if ($rawline =~ /^\+\s* \t\s*\S/ ||
+			    $rawline =~ /^\+\s*        \s*/) {
+				my $herevet = "$here\n" . cat_vet($rawline) . "\n";
+				ERROR("use tabs not spaces\n" . $herevet);
+			}
  		}

  # check for RCS/CVS revision markers

Re: [patch] checkpatch: relax spacing and line length

[Andrew Morton]

On Sun, 6 Apr 2008 06:54:54 +0200 (CEST) Jan Engelhardt <jengelh@computergmbh.de> wrote:

>      We all had the arguments about 80 columns, so here goes a relax.
>      Checking for 95 (or perhaps something better?), but of course we
>      print "80" in the output, because if you happened to get to 95, it's
>      "really time" to break it.

This will reduce the usefulness of checkpatch for those developers who
choose to observe an 80-column limit.

>      This also relaxes the tab doctrine, because spaces DO make sense --
>      especially when you view the code with a tab setting of not-8.

Non-tab-using code inevitably ends up having a mix of tabs and non-tabs
and looks a mess if tabstops are set to anything other than eight.

God I wish I had not been cc'ed on this.

Re: [patch] checkpatch: relax spacing and line length

[Adrian Bunk]

On Sun, Apr 06, 2008 at 06:54:54AM +0200, Jan Engelhardt wrote:
> ===
> total: 0 errors, 0 warnings, 36 lines checked
>
> d27a9f5.diff has no obvious style problems and is ready for submission.
> ===
> commit d27a9f5760fa231ab888f96e27355a001c88b239
> Author: Jan Engelhardt <jengelh@computergmbh.de>
> Date:   Sun Apr 6 06:49:01 2008 +0200
>
>     checkpatch: relax spacing and line length
>
>     We all had the arguments about 80 columns, so here goes a relax.
>     Checking for 95 (or perhaps something better?), but of course we
>     print "80" in the output, because if you happened to get to 95, it's
>     "really time" to break it.

No, it's really time to teach people that checkpatch is *not* a tool for 
janitors.

It's a tool for patch submitters and maintainers that automates a part 
of patch review.

When a patch has a few checkpatch warnings and the submitter can justify 
them (since the code would otherwise look bad) *that is OK*.

But if your driver has over 2000 lines over 80 columns or many lines 
over 95 columns it is not "really time to break it" - it is time to 
check *why* your code has that many that long lines.

>     This also relaxes the tab doctrine, because spaces DO make sense --
>     especially when you view the code with a tab setting of not-8.
>...

In the kernel all tabs are 8 spaces wide.

When you view the code with a different setting that's your fault.

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [patch] checkpatch: relax spacing and line length

[Sam Ravnborg]

> 
> >     This also relaxes the tab doctrine, because spaces DO make sense --
> >     especially when you view the code with a tab setting of not-8.
> >...
> 
> In the kernel all tabs are 8 spaces wide.
> 
> When you view the code with a different setting that's your fault.
And for other kernel developers tabs are for indention, spaces for alignment.

Forget it - the world will not unify about this.

	Sam

Re: [patch] checkpatch: relax spacing and line length

[Benny Halevy]

On Apr. 06, 2008, 8:18 +0300, Andrew Morton <akpm@linux-foundation.org> wrote:
> On Sun, 6 Apr 2008 06:54:54 +0200 (CEST) Jan Engelhardt <jengelh@computergmbh.de> wrote:
> 
>>      We all had the arguments about 80 columns, so here goes a relax.
>>      Checking for 95 (or perhaps something better?), but of course we
>>      print "80" in the output, because if you happened to get to 95, it's
>>      "really time" to break it.
> 
> This will reduce the usefulness of checkpatch for those developers who
> choose to observe an 80-column limit.
> 
>>      This also relaxes the tab doctrine, because spaces DO make sense --
>>      especially when you view the code with a tab setting of not-8.
> 
> Non-tab-using code inevitably ends up having a mix of tabs and non-tabs
> and looks a mess if tabstops are set to anything other than eight.

I humbly disagree.  If you use tabs for (logical) indentation and then trailing
spaces for (graphical) alignment, not just a random mix of tabs and spaces the
code looks fine with 8-character wide tabs or any other setup.  Only when you
use tabs for graphical alignment, below fixed-width characters on the line above
you marry yourself with a specific tab expansion width.

Benny

> 
> God I wish I had not been cc'ed on this.

The idea is *not* to open the flood gates, just to allow for a bit more
flexibility to accommodate for a more robust indentation style.

> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [patch] checkpatch: relax spacing and line length

[Joe Perches]

On Sun, 2008-04-06 at 14:52 +0300, Benny Halevy wrote:
> The idea is [] to allow for a bit more flexibility to
>  accommodate for a more robust indentation style.

Isn't this an editor/tools issue more than a checkpatch one?

I think you should first try to get the most frequently used
code editors and pretty printers (vim/emacs/eclipse/indent, etc)
to support the tabs to level, spaces to align style.

g'luck with that.

Re: [patch] checkpatch: relax spacing and line length

[Boaz Harrosh]

On Sun, Apr 06 2008 at 21:52 +0300, Joe Perches <joe@perches.com> wrote:
> On Sun, 2008-04-06 at 14:52 +0300, Benny Halevy wrote:
>> The idea is [] to allow for a bit more flexibility to
>>  accommodate for a more robust indentation style.
> 
> Isn't this an editor/tools issue more than a checkpatch one?
> 
> I think you should first try to get the most frequently used
> code editors and pretty printers (vim/emacs/eclipse/indent, etc)
> to support the tabs to level, spaces to align style.
> 
> g'luck with that.
> 
> --

I have one such editor to date, Me. But I can not use it's output
because checkpatch.pl forbids it, and I get scorned by my maintainers.
So I think you got it reversed. First allow it then automate for it
other wise there is no use, right?

Boaz

Re: [patch] checkpatch: relax spacing and line length

[Benny Halevy]

On Apr. 06, 2008, 14:08 +0300, Sam Ravnborg <sam@ravnborg.org> wrote:
>>>     This also relaxes the tab doctrine, because spaces DO make sense --
>>>     especially when you view the code with a tab setting of not-8.
>>> ...
>> In the kernel all tabs are 8 spaces wide.
>>
>> When you view the code with a different setting that's your fault.
> And for other kernel developers tabs are for indention, spaces for alignment.
> 
> Forget it - the world will not unify about this.

Resistance to change is only natural, but OTOH no change leads to stagnation.
I completely agree that if we don't encourage this as an alternative it will
indeed never happen but just saying "forget it" brings us nowhere.

I still believe that once people get used to this mentally they can see this
method's merits and how its logic relates to the program's structure: syntax
and coding style.

Benny

> 
> 	Sam
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [patch] checkpatch: relax spacing and line length

[Andy Whitcroft]

On Sun, Apr 06, 2008 at 06:54:54AM +0200, Jan Engelhardt wrote:
> ===
> total: 0 errors, 0 warnings, 36 lines checked
> 
> d27a9f5.diff has no obvious style problems and is ready for submission.
> ===
> commit d27a9f5760fa231ab888f96e27355a001c88b239
> Author: Jan Engelhardt <jengelh@computergmbh.de>
> Date:   Sun Apr 6 06:49:01 2008 +0200
> 
>     checkpatch: relax spacing and line length
> 
>     We all had the arguments about 80 columns, so here goes a relax.
>     Checking for 95 (or perhaps something better?), but of course we
>     print "80" in the output, because if you happened to get to 95, it's
>     "really time" to break it.

Why is it better for the end of the line to sometimes go a bit off the
right of the screen, but not too far?  Are you trying to stop checkpatch
whinging about lines which simply cannot be split pleasently and so poke
a little off, or are you keen to have a bit more space to write code in
as a general rule?

If its the former then you are missing the point of checkpatch, it is
mearly an advisor trying to point those things you have done which the
maintainer is very likely going to notice and going to have to deal with
either by rejecting your patch or fixing it themselves; it is a time
saving aid to them and you.  If the statement really cannot be sanely
wrapped somewhere then do not wrap it.  The maintainer should be able to
see you are correct, if they dissagree they will re-wrap it where they
think it can be sanely wrapped.  While I can imagine that you might have
one or two difficult wraps in a large set of patches, I would not expect
the number of false reports to be significant.  I personally have seen
three or four a year in all the patches I have produced and reviewed.
So it does not seem worth changing the underlying rule just to avoid these
easily ignored reports, expecially considering the number of genuinely
bad lines it would then pass.

If its the latter (you want more space generally), then we should just
say "line length is now N" and be done with it, 95, 128, 200 whatever.
Letting you have 95 characters before telling you should not have had 81
is very non-intuitive and bound to confuse.

>     This also relaxes the tab doctrine, because spaces DO make sense --
>     especially when you view the code with a tab setting of not-8.

This is about visually representing the tabs as smaller units, and still
wanting the rest of the code to line up correctly.  Although I can see
that the effect is somewhat desirable, it feels very much like doing
just this will then go on to encourage the writer to want to violate the
overall line length (as you have more space) and lead to the need to have
more characters on a line in the first place.

For myself I would not necessarily have a problem with this, as I should
be unable to see it in my tabs=8 world.  But unless the code base is
consistant in its use of these then it would seem that their inconsistant
use would distroy the overall effect, and render them ineffective to you
as the consumer.  Also they _will_ get broken by the general populace as
they edit without regard in their tabs=8 world, and their replacements
would be just as acceptable under the new rule as coded.  That would
imply that simply allowing these would not be sufficient, but enforcing
this style (which is much harder) would be required.


As things stand Documentation/CodingStyle is pretty direct in its language
about what is and what is not acceptable in both these areas; I do not
need to run git blame to guess at its author.  It seems entirely reasonable
for checkpatch to implement (as closely as it can) what is carved on that
stone tablet.  The point of having a CodingStyle (and this has been said
many times) is not to please everyone (or indeed anyone but Linus), but to
try and engender consistancy in the code base to ease maintenance for those
higher up the food chain, for those that read all this code.  We all have
our pet styles, and I can assure you Linux kernel style is not my style,
but I write code for the kernel in that style because those are the rules.

To justify changing checkpatch to loosen its checks I would hope to see
an agreed to change to the CodingStyle detailing actually what is now
acceptable.  Failing that strong expressions from maintainers that they
are keen to have code in some alternative style, and presumably _all_
code for their area in that style.  Then it might well make sense to
have different style applied automatically by maintainers area perhaps.
Of course those maintainers would need to be sure their style was going
to be accepted up the chain too.

Comments on the change as it stands follow inline.

>     Signed-off-by: Jan Engelhardt <jengelh@computergmbh.de>
> ---
>  scripts/checkpatch.pl |   22 ++++++++++++++++------
>  1 files changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 58a9494..e5a96c1 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -1094,8 +1094,8 @@ sub process {
>  			my $herevet = "$here\n" . cat_vet($rawline) . "\n";
>  			ERROR("trailing whitespace\n" . $herevet);
>  		}
> -#80 column limit
> -		if ($line =~ /^\+/ && !($prevrawline=~/\/\*\*/) && $length > 
> 80) {
> +#80 column limit (yes, testing for 95 is correct, we do not want to annoy)
> +		if ($line =~ /^\+/ && !($prevrawline=~/\/\*\*/) && $length 
> >= 95) {
>  			WARN("line over 80 characters\n" . $herecurr);

I cannot see how we can fail to confuse our users if we only tell them
they have exceeded 80, when they hit 95.

> @@ -1107,12 +1107,22 @@ sub process {
>  # check we are in a valid source file *.[hc] if not then ignore this hunk
>  		next if ($realfile !~ /\.[hc]$/);
> 
> +		my $arg = qr/$Type(?: ?$Ident)?/;
> +		if ($rawline =~ /^\+ +($arg, )*$arg(?:,|\);?)$/) {
> +			# We are probably in a function parameter list.
> +			# Spaces ok -- used for align.

This seem like it would allow a lot of lines to be aligned using just
spaces.  Even where they are undesirable.

> +		} elsif ($rawline =~ /^\+\t+ *\S/) {
> +			# We are probably in a function or an array/struct
> +			# definition/initializer. Spaces ok -- used for align
> +			# on multiline statements.

This looks very likely to false trigger on any number of unrelated
things.  Almost all lines start with spaces then a non-space character?
Would this not reduce the test to be "you can use any number of spaces
as long as they follow tabs."  And without actually calculating the
indent on the previous line we are not in a position to make any more of
a reasoned check than "\t* *" is ok, else whinge.

Now, we do know the indent to some degree, and we have some feel for
statements, and this align only indent seems only valid "within" a
statement.  So we might well be able to be significantly more targetted.
Indeed were we to want to do this I think that would be required.

> +		} else {
>  # at the beginning of a line any tabs must come first and anything
>  # more than 8 must use tabs.
> -		if ($rawline =~ /^\+\s* \t\s*\S/ ||
> -		    $rawline =~ /^\+\s*        \s*/) {
> -			my $herevet = "$here\n" . cat_vet($rawline) . "\n";
> -			ERROR("use tabs not spaces\n" . $herevet);
> +			if ($rawline =~ /^\+\s* \t\s*\S/ ||
> +			    $rawline =~ /^\+\s*        \s*/) {
> +				my $herevet = "$here\n" . cat_vet($rawline) 
> . "\n";
> +				ERROR("use tabs not spaces\n" . $herevet);
> +			}
>  		}

-apw

Re: [patch] checkpatch: relax spacing and line length

[Andi Kleen]

Andy Whitcroft <apw@shadowen.org> writes:
>
> To justify changing checkpatch to loosen its checks I would hope to see
> an agreed to change to the CodingStyle detailing actually what is now
> acceptable.  

Here was a concrete proposal (with patch for CodingStyle) from my side some 
time ago:

http://article.gmane.org/gmane.linux.kernel/644306

-Andi

Re: [patch] checkpatch: relax spacing and line length

[Andy Whitcroft]

On Tue, Apr 08, 2008 at 08:01:14PM +0200, Andi Kleen wrote:
> Andy Whitcroft <apw@shadowen.org> writes:
> >
> > To justify changing checkpatch to loosen its checks I would hope to see
> > an agreed to change to the CodingStyle detailing actually what is now
> > acceptable.  
> 
> Here was a concrete proposal (with patch for CodingStyle) from my side some 
> time ago:
> 
> http://article.gmane.org/gmane.linux.kernel/644306

Yeah, that was a good example of how to go about changinging things.  As
I said on that thread I would happily change checkpatch to follow suit.
Oddly for a CodingStyle discussion there was very "discussion" at all.
It feels like people are scared of the passion that often errupts in
discussions over style.

On that particular suggestion, I would probabally be in favour, and
slightly happier to have the string on the printk line for consistency
even though that would violate the "don't hide information" rule.

-apw

Re: [patch] checkpatch: relax spacing and line length

[Andy Whitcroft]

On Wed, Apr 09, 2008 at 09:19:43AM +0100, Andy Whitcroft wrote:
> On Tue, Apr 08, 2008 at 08:01:14PM +0200, Andi Kleen wrote:
> > Andy Whitcroft <apw@shadowen.org> writes:
> > >
> > > To justify changing checkpatch to loosen its checks I would hope to see
> > > an agreed to change to the CodingStyle detailing actually what is now
> > > acceptable.  
> > 
> > Here was a concrete proposal (with patch for CodingStyle) from my side some 
> > time ago:
> > 
> > http://article.gmane.org/gmane.linux.kernel/644306
> 
> Yeah, that was a good example of how to go about changinging things.  As
> I said on that thread I would happily change checkpatch to follow suit.
> Oddly for a CodingStyle discussion there was very "discussion" at all.
> It feels like people are scared of the passion that often errupts in
> discussions over style.
> 
> On that particular suggestion, I would probabally be in favour, and
> slightly happier to have the string on the printk line for consistency
> even though that would violate the "don't hide information" rule.

Oh, and if people felt that the concensus was for something to be
implemented and that you are waiting for me to implement the change in
checkpatch; please say so.

-apw

Re: [patch] checkpatch: relax spacing and line length

[Andi Kleen]

> Oh, and if people felt that the concensus was for something to be
> implemented and that you are waiting for me to implement the change in
> checkpatch; please say so.

Well at least I think the printk change is a good one to implement and there 
wasn't much protest to it at least.

If you're looking for another change request I think dropping the trailing
white space check would be good a idea because a lot of maintainers already
handle that automatically and it is not really worth bothering the "leaf
developers" with because that can be trivially automated.

Also I still think --file needs to go, or at least be replaced with
--file-force and warning. See the recent unpleasant incident it caused
again, e.g.

http://thread.gmane.org/gmane.linux.kernel/656847/focus=657003
and 
http://thread.gmane.org/gmane.linux.kernel/656847/focus=657003

-Andi

Re: [patch] checkpatch: relax spacing and line length

[Benny Halevy]

On Apr. 08, 2008, 20:12 +0300, Andy Whitcroft <apw@shadowen.org> wrote:
> On Sun, Apr 06, 2008 at 06:54:54AM +0200, Jan Engelhardt wrote:
>> ===
>> total: 0 errors, 0 warnings, 36 lines checked
>>
>> d27a9f5.diff has no obvious style problems and is ready for submission.
>> ===
>> commit d27a9f5760fa231ab888f96e27355a001c88b239
>> Author: Jan Engelhardt <jengelh@computergmbh.de>
>> Date:   Sun Apr 6 06:49:01 2008 +0200
>>
>>     checkpatch: relax spacing and line length
>>
>>     We all had the arguments about 80 columns, so here goes a relax.
>>     Checking for 95 (or perhaps something better?), but of course we
>>     print "80" in the output, because if you happened to get to 95, it's
>>     "really time" to break it.
> 
> Why is it better for the end of the line to sometimes go a bit off the
> right of the screen, but not too far?  Are you trying to stop checkpatch
> whinging about lines which simply cannot be split pleasently and so poke
> a little off, or are you keen to have a bit more space to write code in
> as a general rule?
> 
> If its the former then you are missing the point of checkpatch, it is
> mearly an advisor trying to point those things you have done which the
> maintainer is very likely going to notice and going to have to deal with
> either by rejecting your patch or fixing it themselves; it is a time
> saving aid to them and you.  If the statement really cannot be sanely
> wrapped somewhere then do not wrap it.  The maintainer should be able to
> see you are correct, if they dissagree they will re-wrap it where they
> think it can be sanely wrapped.  While I can imagine that you might have
> one or two difficult wraps in a large set of patches, I would not expect
> the number of false reports to be significant.  I personally have seen
> three or four a year in all the patches I have produced and reviewed.
> So it does not seem worth changing the underlying rule just to avoid these
> easily ignored reports, expecially considering the number of genuinely
> bad lines it would then pass.
> 
> If its the latter (you want more space generally), then we should just
> say "line length is now N" and be done with it, 95, 128, 200 whatever.
> Letting you have 95 characters before telling you should not have had 81
> is very non-intuitive and bound to confuse.
> 
>>     This also relaxes the tab doctrine, because spaces DO make sense --
>>     especially when you view the code with a tab setting of not-8.
> 
> This is about visually representing the tabs as smaller units, and still
> wanting the rest of the code to line up correctly.  Although I can see
> that the effect is somewhat desirable, it feels very much like doing
> just this will then go on to encourage the writer to want to violate the
> overall line length (as you have more space) and lead to the need to have
> more characters on a line in the first place.

As I said on a different thread, tabs should still be accounted for as
8 spaces for checking the total line length.  The motivation is to
keep lines shorter and limit nesting.

> 
> For myself I would not necessarily have a problem with this, as I should
> be unable to see it in my tabs=8 world.  But unless the code base is
> consistant in its use of these then it would seem that their inconsistant
> use would distroy the overall effect, and render them ineffective to you
> as the consumer.  Also they _will_ get broken by the general populace as
> they edit without regard in their tabs=8 world, and their replacements
> would be just as acceptable under the new rule as coded.  That would
> imply that simply allowing these would not be sufficient, but enforcing
> this style (which is much harder) would be required.
> 
> 
> As things stand Documentation/CodingStyle is pretty direct in its language
> about what is and what is not acceptable in both these areas; I do not
> need to run git blame to guess at its author.  It seems entirely reasonable
> for checkpatch to implement (as closely as it can) what is carved on that
> stone tablet.  The point of having a CodingStyle (and this has been said
> many times) is not to please everyone (or indeed anyone but Linus), but to
> try and engender consistancy in the code base to ease maintenance for those
> higher up the food chain, for those that read all this code.  We all have
> our pet styles, and I can assure you Linux kernel style is not my style,
> but I write code for the kernel in that style because those are the rules.
> 
> To justify changing checkpatch to loosen its checks I would hope to see
> an agreed to change to the CodingStyle detailing actually what is now
> acceptable.  Failing that strong expressions from maintainers that they
> are keen to have code in some alternative style, and presumably _all_
> code for their area in that style.  Then it might well make sense to
> have different style applied automatically by maintainers area perhaps.
> Of course those maintainers would need to be sure their style was going
> to be accepted up the chain too.
> 
> Comments on the change as it stands follow inline.
> 
>>     Signed-off-by: Jan Engelhardt <jengelh@computergmbh.de>
>> ---
>>  scripts/checkpatch.pl |   22 ++++++++++++++++------
>>  1 files changed, 16 insertions(+), 6 deletions(-)
>>
>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
>> index 58a9494..e5a96c1 100755
>> --- a/scripts/checkpatch.pl
>> +++ b/scripts/checkpatch.pl
>> @@ -1094,8 +1094,8 @@ sub process {
>>  			my $herevet = "$here\n" . cat_vet($rawline) . "\n";
>>  			ERROR("trailing whitespace\n" . $herevet);
>>  		}
>> -#80 column limit
>> -		if ($line =~ /^\+/ && !($prevrawline=~/\/\*\*/) && $length > 
>> 80) {
>> +#80 column limit (yes, testing for 95 is correct, we do not want to annoy)
>> +		if ($line =~ /^\+/ && !($prevrawline=~/\/\*\*/) && $length 
>>> = 95) {
>>  			WARN("line over 80 characters\n" . $herecurr);
> 
> I cannot see how we can fail to confuse our users if we only tell them
> they have exceeded 80, when they hit 95.

I agree.

> 
>> @@ -1107,12 +1107,22 @@ sub process {
>>  # check we are in a valid source file *.[hc] if not then ignore this hunk
>>  		next if ($realfile !~ /\.[hc]$/);
>>
>> +		my $arg = qr/$Type(?: ?$Ident)?/;
>> +		if ($rawline =~ /^\+ +($arg, )*$arg(?:,|\);?)$/) {
>> +			# We are probably in a function parameter list.
>> +			# Spaces ok -- used for align.
> 
> This seem like it would allow a lot of lines to be aligned using just
> spaces.  Even where they are undesirable.
> 
>> +		} elsif ($rawline =~ /^\+\t+ *\S/) {
>> +			# We are probably in a function or an array/struct
>> +			# definition/initializer. Spaces ok -- used for align
>> +			# on multiline statements.
> 
> This looks very likely to false trigger on any number of unrelated
> things.  Almost all lines start with spaces then a non-space character?
> Would this not reduce the test to be "you can use any number of spaces
> as long as they follow tabs."  And without actually calculating the
> indent on the previous line we are not in a position to make any more of
> a reasoned check than "\t* *" is ok, else whinge.

Yeah, I think that this was Jan's intention.

> 
> Now, we do know the indent to some degree, and we have some feel for
> statements, and this align only indent seems only valid "within" a
> statement.  So we might well be able to be significantly more targetted.
> Indeed were we to want to do this I think that would be required.

It'd be great if we could verify that the number of tabs is equal or
greater than the nesting level (and then followed by zero or more spaces).

This brings into mind if we could/should warn about excessive nesting?
I think it might be a good idea, though I'm not sure what would be
the threshold... Off the top of my head I'd say "around 5".

> 
>> +		} else {
>>  # at the beginning of a line any tabs must come first and anything
>>  # more than 8 must use tabs.
>> -		if ($rawline =~ /^\+\s* \t\s*\S/ ||
>> -		    $rawline =~ /^\+\s*        \s*/) {
>> -			my $herevet = "$here\n" . cat_vet($rawline) . "\n";
>> -			ERROR("use tabs not spaces\n" . $herevet);
>> +			if ($rawline =~ /^\+\s* \t\s*\S/ ||
>> +			    $rawline =~ /^\+\s*        \s*/) {
>> +				my $herevet = "$here\n" . cat_vet($rawline) 
>> . "\n";
>> +				ERROR("use tabs not spaces\n" . $herevet);
>> +			}
>>  		}
> 
> -apw
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [patch] checkpatch: relax spacing and line length

[Benny Halevy]

On Apr. 08, 2008, 20:12 +0300, Andy Whitcroft <apw@shadowen.org> wrote:
> To justify changing checkpatch to loosen its checks I would hope to see
> an agreed to change to the CodingStyle detailing actually what is now
> acceptable.

For reference, here's Jan's proposal for Documentation/CodingStyle:
http://lkml.org/lkml/2008/2/26/462

Benny

Re: [patch] checkpatch: relax spacing and line length

[Andy Whitcroft]

On Wed, Apr 09, 2008 at 10:46:06AM +0200, Andi Kleen wrote:
> > Oh, and if people felt that the concensus was for something to be
> > implemented and that you are waiting for me to implement the change in
> > checkpatch; please say so.
> 
> Well at least I think the printk change is a good one to implement and there 
> wasn't much protest to it at least.

Ok.  will put this on my todo list.

> If you're looking for another change request I think dropping the trailing
> white space check would be good a idea because a lot of maintainers already
> handle that automatically and it is not really worth bothering the "leaf
> developers" with because that can be trivially automated.

I assume you are talking about git squashing bad line ends as it loads
the patches up.  I thought that this still triggered issues when loading
subsequent patches which relied on the context including those spaces.
Surely, this would lead to increased load on the maintainers as they
loaded those patches up?  Where the leaf maintainers use quilt would they
either not get seen or trigger failurs for him (dependant on whether he
has the option enabled), both bad outcomes.  Better to waste the time of
leaf contributers (such as myself) than any maintainers time, IMO.

> Also I still think --file needs to go, or at least be replaced with
> --file-force and warning. See the recent unpleasant incident it caused
> again, e.g.
> 
> http://thread.gmane.org/gmane.linux.kernel/656847/focus=657003
> and 
> http://thread.gmane.org/gmane.linux.kernel/656847/focus=657003

Ok, these are both the same link, so either you miss pasted one, or you
repeat to emphasise.

Reading over that thread there seems to be a few themes:

1) "don't send out 150 patches at once",
2) accepting checkpatch only changes is stupid, and
3) some maintainers do not like the style checkpatch recommends.

The first is really a general complaint against the originator, not really
a complaint about checkpatch.  Checkpatch may have prompted the changes but
that is not ultimatly its fault.  We may not like a huge pile of patches
dropped at once, but failure to recognise that is a failing in education,
not a tooling issue.  Also with the huge volumes on linux-kernel these
are hardly a major component of overall volumes?

The second is actually a matter for maintainers, either they will accept
checkpatch only changes or they won't.  Clearly Dave Miller and Andi
will not, clearly Ingo will.  Surly that is a matter for the Maintainer,
and not for the tool.

The third is about some maintainers wishing to use different style for
their parts of their tree.  That is their right, assuming those upstream
of them will accept their style.


There is a lot of polorised view on checkpatch.  But clearly there are
maintainers on both sides of the argument, those who find the tool
helps them and those who do not.  Maintainers who believe that they
should police the coding style, and find that the tool helps them with
that part of the process reducing their burden when reviewing submission.
Others who think its crap and would rather visually check style themselves,
or indeed ignore style.

On the --force-file, we tried making the warning occur in the tool itself
once before and that produced extremely vocal opposition, flame fests,
and general unhappyness, so I don't really see that flying here.  What I
do not really understand is why a simple maintainer rejection of such
patches would not work just as well, "I do not accept checkpatch style
only cleanups".  As things stand a chunk of those changes were picked up,
others implicitly rejected; lesson learned for the submitter surely.

I do feel that it may be helpful to update SubmittingPatches to mention
the problems introduced by style only churn, and (as we have a difference
of opinion) to recommend contacting the area Maintainer before embarking
on such things.  Something like the patch below perhaps?

-apw

=== 8< ===
add a note on the risks associated with wide ranging style cleanups

Add a NOTE to the SubmittingPatches guide pointing out the issues which
may occur when submitting wide ranging style cleanups, and pointing the
reader to the area maintainers.

Signed-off-by: Andy Whitcroft <apw@shadowen.org>
---
 Documentation/SubmittingPatches |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)
diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
index 08a1ed1..633b9cf 100644
--- a/Documentation/SubmittingPatches
+++ b/Documentation/SubmittingPatches
@@ -472,6 +472,12 @@ The checker reports at three levels:
 You should be able to justify all violations that remain in your
 patch.
 
+NOTE: updating entire files purely for style violations can cause
+significant issues for other contributers and maintainers by increasing
+collisions between patches.  It is therefore recommended you contact the
+area Maintainer to ensure these patches are likely to be accepted _before_
+starting work.
+
 
 
 2) #ifdefs are ugly

Re: [patch] checkpatch: relax spacing and line length

[Jan Engelhardt]

On Wednesday 2008-04-09 15:14, Andy Whitcroft wrote:
>On Wed, Apr 09, 2008 at 10:46:06AM +0200, Andi Kleen wrote:
>> > Oh, and if people felt that the concensus was for something to be
>> > implemented and that you are waiting for me to implement the change in
>> > checkpatch; please say so.
>> 
>> Well at least I think the printk change is a good one to implement and there 
>> wasn't much protest to it at least.
>
>Ok.  will put this on my todo list.

Instead of

	if (foo) {
		if (baz) {
			++x;
printk("Oh so long line makes my coding style go wary... nonsensical sentence\n");
		}
	}

I'd keep the indent and allow elongated lines:

	if (foo) {
		if (baz) {
			++x;
			printk("Oh so long line makes my coding style go wary... nonsensical sentence\n");
		}
	}

Or perhaps you just pointed out we need a smarter grep program! :)

Re: [patch] checkpatch: relax spacing and line length

[Andy Whitcroft]

On Wed, Apr 09, 2008 at 03:19:36PM +0300, Benny Halevy wrote:
> On Apr. 08, 2008, 20:12 +0300, Andy Whitcroft <apw@shadowen.org> wrote:
> > To justify changing checkpatch to loosen its checks I would hope to see
> > an agreed to change to the CodingStyle detailing actually what is now
> > acceptable.
> 
> For reference, here's Jan's proposal for Documentation/CodingStyle:
> http://lkml.org/lkml/2008/2/26/462

Yes, seems reasonably well worded.  However, I see no consensus for its
acceptance as a change.  I seem some near NAK's.

-apw

Re: [patch] checkpatch: relax spacing and line length

[Andy Whitcroft]

On Wed, Apr 09, 2008 at 03:18:47PM +0200, Jan Engelhardt wrote:
> 
> On Wednesday 2008-04-09 15:14, Andy Whitcroft wrote:
> >On Wed, Apr 09, 2008 at 10:46:06AM +0200, Andi Kleen wrote:
> >> > Oh, and if people felt that the concensus was for something to be
> >> > implemented and that you are waiting for me to implement the change in
> >> > checkpatch; please say so.
> >> 
> >> Well at least I think the printk change is a good one to implement and there 
> >> wasn't much protest to it at least.
> >
> >Ok.  will put this on my todo list.
> 
> Instead of
> 
> 	if (foo) {
> 		if (baz) {
> 			++x;
> printk("Oh so long line makes my coding style go wary... nonsensical sentence\n");
> 		}
> 	}
> 
> I'd keep the indent and allow elongated lines:
> 
> 	if (foo) {
> 		if (baz) {
> 			++x;
> 			printk("Oh so long line makes my coding style go wary... nonsensical sentence\n");
> 		}
> 	}
> 
> Or perhaps you just pointed out we need a smarter grep program! :)


My preference would be for the latter.  Keep the line indent consistent
and allow the line to overspill.  But that would depend on the concensus
obviously.  The originally suggested layout was:

			printk(
"Oh ....",
				a, b);

-apw

Re: [patch] checkpatch: relax spacing and line length

[Randy Dunlap]

On Wed, 9 Apr 2008 14:14:20 +0100 Andy Whitcroft wrote:

> === 8< ===
> add a note on the risks associated with wide ranging style cleanups
> 
> Add a NOTE to the SubmittingPatches guide pointing out the issues which
> may occur when submitting wide ranging style cleanups, and pointing the
> reader to the area maintainers.
> 
> Signed-off-by: Andy Whitcroft <apw@shadowen.org>
> ---
>  Documentation/SubmittingPatches |    6 ++++++
>  1 files changed, 6 insertions(+), 0 deletions(-)
> diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
> index 08a1ed1..633b9cf 100644
> --- a/Documentation/SubmittingPatches
> +++ b/Documentation/SubmittingPatches
> @@ -472,6 +472,12 @@ The checker reports at three levels:
>  You should be able to justify all violations that remain in your
>  patch.
>  
> +NOTE: updating entire files purely for style violations can cause
> +significant issues for other contributers and maintainers by increasing

                                contributors

> +collisions between patches.  It is therefore recommended you contact the
> +area Maintainer to ensure these patches are likely to be accepted _before_
> +starting work.
> +
>  
>  
>  2) #ifdefs are ugly



---
~Randy

Re: [patch] checkpatch: relax spacing and line length

[Andrew Morton]

On Wed, 9 Apr 2008 14:58:40 +0100 Andy Whitcroft <apw@shadowen.org> wrote:

> The originally suggested layout was:
> 
> 			printk(
> "Oh ....",
> 				a, b);

Dunno about others, but when I see that idiom I fall over stunned and 
can't get up again.

Re: [patch] checkpatch: relax spacing and line length

[Benny Halevy]

On Apr. 09, 2008, 16:25 +0300, Andy Whitcroft <apw@shadowen.org> wrote:
> On Wed, Apr 09, 2008 at 03:19:36PM +0300, Benny Halevy wrote:
>> On Apr. 08, 2008, 20:12 +0300, Andy Whitcroft <apw@shadowen.org> wrote:
>>> To justify changing checkpatch to loosen its checks I would hope to see
>>> an agreed to change to the CodingStyle detailing actually what is now
>>> acceptable.
>> For reference, here's Jan's proposal for Documentation/CodingStyle:
>> http://lkml.org/lkml/2008/2/26/462
> 
> Yes, seems reasonably well worded.  However, I see no consensus for its
> acceptance as a change.  I seem some near NAK's.

but no definite one :)
> 
> -apw

Seriously, I'm not sure how significant or relevant they are though.

In http://lkml.org/lkml/2008/2/26/533,
SL Baur <steve <at> xemacs.org> said:
> The proposed two space change is ugly.  Can someone NAK it?

I'm not sure what "two space change" proposal this Steve referred to
and his rejection is based on not-to-sound aesthetic grounds.

The motivation behind our proposal is more than just aesthetic.
I believe that using tabs for indent and then spaces for alignment
is functionally better, works for everybody, and will eventually result
in a more readable code over time, hopefully leading to fewer bugs.


Randy's answer, http://lkml.org/lkml/2008/2/27/7
says he won't NAK it since:
> I would gladly NAK it, but most recent email from Linus about
> coding style is that we are getting too detailed about it,
> so unless there is some overwhelming need to change anything in
> CodingStyle, I'm for no changes (or maybe even some removals).

My interpretation of that is the the current CodingStyle is too detailed
*now* therefore we need to relax it, not keep it the way it is.
It's true, that we add more details to relax the requirements but
overall we'd allow for more flexibility.  To do that with removing
details rather than adding any is dangerous IMO since it can easily
lead to indentation chaos that makes everybody's life harder...

Richard Knutsson, in http://lkml.org/lkml/2008/2/28/356
adds an excellent point about needing smaller tab expansion
for narrow screens.

Stefan Richter in http://lkml.org/lkml/2008/2/26/523 commented:
> Jan Engelhardt, Benny Halevy, and Richard Knutsson wrote:
> > -Tabs are 8 characters, and thus indentations are also 8 characters.
> > -There are heretic movements that try to make indentations 4 (or even 2!)
> > -characters deep, and that is akin to trying to define the value of PI to
> > -be 3.
> 
> Don't do this

Again, I see no real reasons why not to besides being against Stefan's
preferences.  I repeat my point that the proposed style does not
necessarily encourage smaller tab expansion, it just makes it possible.

Well, enough said.
Back to fixin' bugs...

Benny

Re: [patch] checkpatch: relax spacing and line length

[Stefan Richter]

Benny Halevy wrote:
> Stefan Richter in http://lkml.org/lkml/2008/2/26/523 commented:
>> Jan Engelhardt, Benny Halevy, and Richard Knutsson wrote:
>> > -Tabs are 8 characters, and thus indentations are also 8 characters.
>> > -There are heretic movements that try to make indentations 4 (or even 2!)
>> > -characters deep, and that is akin to trying to define the value of PI to
>> > -be 3.
>> 
>> Don't do this
> 
> Again, I see no real reasons why not to besides being against Stefan's
> preferences.

BTW, my preference was about keeping the last traces of witty language
in this text, not about any particular whitespace language.

(Do you have an idea who wrote the sentences which that patch wanted to
delete, and more importantly, *why* he wrote it this way?  You
apparently don't yet, but maybe you think about it once more.  Thanks.)
-- 
Stefan Richter
-=====-==--- -=-- -=--=
http://arcgraph.de/sr/

Re: [patch] checkpatch: relax spacing and line length

[Andi Kleen]

On Wed, Apr 09, 2008 at 09:53:26AM -0700, Andrew Morton wrote:
> On Wed, 9 Apr 2008 14:58:40 +0100 Andy Whitcroft <apw@shadowen.org> wrote:
> 
> > The originally suggested layout was:
> > 
> > 			printk(
> > "Oh ....",
> > 				a, b);
> 
> Dunno about others, but when I see that idiom I fall over stunned and 
> can't get up again.

Not sure what the problem is. But my original proposal had two alternatives.

Either unindent (as above) or exceed 80 characters for the format string as 
needed to keep it together. 

I've used both in the past and don't have a particular preference.

-Andi

Re: [patch] checkpatch: relax spacing and line length

[Andy Whitcroft]

On Wed, Apr 09, 2008 at 09:53:26AM -0700, Andrew Morton wrote:
> On Wed, 9 Apr 2008 14:58:40 +0100 Andy Whitcroft <apw@shadowen.org> wrote:
> 
> > The originally suggested layout was:
> > 
> > 			printk(
> > "Oh ....",
> > 				a, b);
> 
> Dunno about others, but when I see that idiom I fall over stunned and 
> can't get up again.

That remains me of nethack.  I'll start with the width breaker.

-apw

Re: [patch] checkpatch: relax spacing and line length

[Andy Whitcroft]

On Wed, Apr 09, 2008 at 08:02:39PM +0300, Benny Halevy wrote:
> On Apr. 09, 2008, 16:25 +0300, Andy Whitcroft <apw@shadowen.org> wrote:
> > On Wed, Apr 09, 2008 at 03:19:36PM +0300, Benny Halevy wrote:
> >> On Apr. 08, 2008, 20:12 +0300, Andy Whitcroft <apw@shadowen.org> wrote:
> >>> To justify changing checkpatch to loosen its checks I would hope to see
> >>> an agreed to change to the CodingStyle detailing actually what is now
> >>> acceptable.
> >> For reference, here's Jan's proposal for Documentation/CodingStyle:
> >> http://lkml.org/lkml/2008/2/26/462
> > 
> > Yes, seems reasonably well worded.  However, I see no consensus for its
> > acceptance as a change.  I seem some near NAK's.
> 
> but no definite one :)
> > 
> > -apw
> 
> Seriously, I'm not sure how significant or relevant they are though.
> 
> In http://lkml.org/lkml/2008/2/26/533,
> SL Baur <steve <at> xemacs.org> said:
> > The proposed two space change is ugly.  Can someone NAK it?
> 
> I'm not sure what "two space change" proposal this Steve referred to
> and his rejection is based on not-to-sound aesthetic grounds.
> 
> The motivation behind our proposal is more than just aesthetic.
> I believe that using tabs for indent and then spaces for alignment
> is functionally better, works for everybody, and will eventually result
> in a more readable code over time, hopefully leading to fewer bugs.

I don't see it as really anything other than different.  It'll look better
for you, sure.  But either we (tabs=8 people) will not be maintaining it
as we edit leading to inconsistent indent, or we will be putting in lots
of effort to maintain something we can't even see.

> Randy's answer, http://lkml.org/lkml/2008/2/27/7
> says he won't NAK it since:
> > I would gladly NAK it, but most recent email from Linus about
> > coding style is that we are getting too detailed about it,
> > so unless there is some overwhelming need to change anything in
> > CodingStyle, I'm for no changes (or maybe even some removals).
> 
> My interpretation of that is the the current CodingStyle is too detailed
> *now* therefore we need to relax it, not keep it the way it is.
> It's true, that we add more details to relax the requirements but
> overall we'd allow for more flexibility.  To do that with removing
> details rather than adding any is dangerous IMO since it can easily
> lead to indentation chaos that makes everybody's life harder...

But that is your option.  Right now the rule is simple, use tabs, only.
Either we relax that and get inconsistent indent from the two camps _or_
you add rules and we all have to follow your new spaces for align form.

> Richard Knutsson, in http://lkml.org/lkml/2008/2/28/356
> adds an excellent point about needing smaller tab expansion
> for narrow screens.
> 
> Stefan Richter in http://lkml.org/lkml/2008/2/26/523 commented:
> > Jan Engelhardt, Benny Halevy, and Richard Knutsson wrote:
> > > -Tabs are 8 characters, and thus indentations are also 8 characters.
> > > -There are heretic movements that try to make indentations 4 (or even 2!)
> > > -characters deep, and that is akin to trying to define the value of PI to
> > > -be 3.
> > 
> > Don't do this
> 
> Again, I see no real reasons why not to besides being against Stefan's
> preferences.  I repeat my point that the proposed style does not
> necessarily encourage smaller tab expansion, it just makes it possible.
> 
> Well, enough said.
> Back to fixin' bugs...

Indeed.

-apw

Re: [patch] checkpatch: relax spacing and line length

[SL Baur]

On 4/9/08, Benny Halevy <bhalevy@panasas.com> wrote:

>  In http://lkml.org/lkml/2008/2/26/533,
>  SL Baur <steve <at> xemacs.org> said:
>  > The proposed two space change is ugly.  Can someone NAK it?
>
>  I'm not sure what "two space change" proposal this Steve referred to
>  and his rejection is based on not-to-sound aesthetic grounds.
>
>  The motivation behind our proposal is more than just aesthetic.
>  I believe that using tabs for indent and then spaces for alignment
>  is functionally better, works for everybody, and will eventually result
>  in a more readable code over time, hopefully leading to fewer bugs.

Tabs + a variable number spaces to match up logically with the
previous line is O.K.  Tabs + exactly two spaces is what I objected
to.  I only posted due to being the originator of the Linux Kernel Emacs
cc-mode style and cc-mode works like the former.

For whatever it's worth, my sentiments are on the David Miller side,
having been the lucky recipient of format changes only patch bombs
before.  I suppose you should be glad you don't have someone running
a spell checker on the entire source code.

-sb

Re: [patch] checkpatch: relax spacing and line length

[Jan Engelhardt]

On Friday 2008-04-11 01:52, SL Baur wrote:
>On 4/9/08, Benny Halevy <bhalevy@panasas.com> wrote:
>
>>  In http://lkml.org/lkml/2008/2/26/533,
>>  SL Baur <steve <at> xemacs.org> said:
>>  > The proposed two space change is ugly.  Can someone NAK it?
>>
>>  I'm not sure what "two space change" proposal this Steve referred to
>>  and his rejection is based on not-to-sound aesthetic grounds.
>>
>>  The motivation behind our proposal is more than just aesthetic.
>>  I believe that using tabs for indent and then spaces for alignment
>>  is functionally better, works for everybody, and will eventually result
>>  in a more readable code over time, hopefully leading to fewer bugs.
>
>Tabs + a variable number spaces to match up logically with the
>previous line is O.K.  Tabs + exactly two spaces is what I objected
>to.  I only posted due to being the originator of the Linux Kernel Emacs
>cc-mode style and cc-mode works like the former.
>
>For whatever it's worth, my sentiments are on the David Miller side,
>having been the lucky recipient of format changes only patch bombs
>before.  I suppose you should be glad you don't have someone running
>a spell checker on the entire source code.

We have these people too. And by large, I find spell fixes
better than oh-I-decided-to-run-checkpatch-on-existing-code bombs.

Re: [patch] checkpatch: relax spacing and line length

[Andy Whitcroft]

On Wed, Apr 09, 2008 at 09:07:55PM +0100, Andy Whitcroft wrote:
> On Wed, Apr 09, 2008 at 09:53:26AM -0700, Andrew Morton wrote:
> > On Wed, 9 Apr 2008 14:58:40 +0100 Andy Whitcroft <apw@shadowen.org> wrote:
> > 
> > > The originally suggested layout was:
> > > 
> > > 			printk(
> > > "Oh ....",
> > > 				a, b);
> > 
> > Dunno about others, but when I see that idiom I fall over stunned and 
> > can't get up again.
> 
> That remains me of nethack.  I'll start with the width breaker.

Ok.  I've just pushed out a testing version of checkpatch which is
identicle the 0.18 delta I just pushed to Andrew with the printk width
restriction lifted.  Perhaps those interested might test it to see if it
whines appropriatly:

  http://www.kernel.org/pub/linux/kernel/people/apw/checkpatch/checkpatch.pl-testing

I am going to be off next week, but will likely be dipping into email
now and again.

-apw

Re: [patch] checkpatch: relax spacing and line length

[Al Viro]

On Wed, Apr 09, 2008 at 08:02:39PM +0300, Benny Halevy wrote:

> I'm not sure what "two space change" proposal this Steve referred to
> and his rejection is based on not-to-sound aesthetic grounds.
> 
> The motivation behind our proposal is more than just aesthetic.
> I believe that using tabs for indent and then spaces for alignment
> is functionally better, works for everybody, and will eventually result
> in a more readable code over time, hopefully leading to fewer bugs.

Sorry.  I'm not going to change perfectly working editing habits *or* to patch
nvi to satisfy an annoying wunch of bankers.  HAND, GAFL.

Re: [patch] checkpatch: relax spacing and line length

[Benny Halevy]

On Apr. 12, 2008, 3:26 +0300, Al Viro <viro@ZenIV.linux.org.uk> wrote:
> On Wed, Apr 09, 2008 at 08:02:39PM +0300, Benny Halevy wrote:
> 
>> I'm not sure what "two space change" proposal this Steve referred to
>> and his rejection is based on not-to-sound aesthetic grounds.
>>
>> The motivation behind our proposal is more than just aesthetic.
>> I believe that using tabs for indent and then spaces for alignment
>> is functionally better, works for everybody, and will eventually result
>> in a more readable code over time, hopefully leading to fewer bugs.
> 
> Sorry.  I'm not going to change perfectly working editing habits *or* to patch

That's working well for you but apparently not so well for everybody else.

> nvi to satisfy an annoying wunch of bankers.  HAND, GAFL.
> --

Thanks for the insightful and mature comment, Al.

I really hate to spend more time on this topic but folks did find merits in it.
There's no need to change anybody's editing habits if we allow this indentation
style in the CodingStyle document and in checkpatch.pl in addition to the
existing convention.

Benny

Re: [patch] checkpatch: relax spacing and line length

[Al Viro]

On Sun, Apr 13, 2008 at 12:53:48PM +0300, Benny Halevy wrote:

> > Sorry.  I'm not going to change perfectly working editing habits *or* to patch
> 
> That's working well for you but apparently not so well for everybody else.

Nice turn of a phrase, that.

> > nvi to satisfy an annoying wunch of bankers.  HAND, GAFL.
> > --
> 
> Thanks for the insightful and mature comment, Al.

Oh, for the...

> I really hate to spend more time on this topic but folks did find merits in it.
> There's no need to change anybody's editing habits if we allow this indentation
> style in the CodingStyle document and in checkpatch.pl in addition to the
> existing convention.

"Allow" is such a nice word, isn't it?  Let's take a closer look:
	* nobody prohibits lines satisfying your constraints ("tabs only for
indent level"), so "allowing" that is meaningless
	* "indentation style" in the above refers to editor settings.
To "allow" that, you advocate prohibiting the lines _NOT_ satisfying your
constraints.  Which, by definition, means extra work for people submitting
patches, no matter how you spin it.

BTW, while we are talking about conventions, would you mind keeping lines
in your mail shorter than 79 columns to avoid wraparounds in quoted text?
Unlike your proposal, that one actually _is_ a common convention...

Re: [patch] checkpatch: relax spacing and line length

[Benny Halevy]

On Apr. 13, 2008, 18:18 +0300, Al Viro <viro@ZenIV.linux.org.uk> wrote:
> On Sun, Apr 13, 2008 at 12:53:48PM +0300, Benny Halevy wrote:
> 
>>> Sorry.  I'm not going to change perfectly working editing habits *or* to patch
>> That's working well for you but apparently not so well for everybody else.
> 
> Nice turn of a phrase, that.
> 
>>> nvi to satisfy an annoying wunch of bankers.  HAND, GAFL.
>>> --
>> Thanks for the insightful and mature comment, Al.
> 
> Oh, for the...
> 
>> I really hate to spend more time on this topic but folks did find merits in it.
>> There's no need to change anybody's editing habits if we allow this indentation
>> style in the CodingStyle document and in checkpatch.pl in addition to the
>> existing convention.
> 
> "Allow" is such a nice word, isn't it?  Let's take a closer look:
> 	* nobody prohibits lines satisfying your constraints ("tabs only for
> indent level"), so "allowing" that is meaningless

Currently checkpatch.pl prints an error if I use 8 or more spaces in the
indentation string and Documentation/CodingStyle says:

"Outside of comments, documentation and except in Kconfig, spaces are never
used for indentation"

Although CodingStyle and checkpatch just provide guidance and the final
word is the maintainer's I consider these recommendations as "disallowing",
or at least "discouraging". So did others that commented on patches I sent
in the past.  If that wasn't the case I wouldn't have come up with this
silly initiative in the first place.

> 	* "indentation style" in the above refers to editor settings.
> To "allow" that, you advocate prohibiting the lines _NOT_ satisfying your
> constraints.  Which, by definition, means extra work for people submitting
> patches, no matter how you spin it.

I'm certainly not advocating to prohibit the current indentation style,
just to relax the rules to allow a superset of it.

Basically, I'd like checkpatch to allow /^\+\t* *\S/
and, since Andy says that checkpatch knows "the indent to some degree",
it can warn if the number of leading tabs is smaller than that.

> 
> BTW, while we are talking about conventions, would you mind keeping lines
> in your mail shorter than 79 columns to avoid wraparounds in quoted text?
> Unlike your proposal, that one actually _is_ a common convention...

No, I don't mind.

[Though it is a bit of a pain to keep that when automatic wrapping of long
lines is turned off in my mail program so I can easily quote patches or
code snippets.]

Benny