Message-ID: <20080408171257.GF17915@shadowen.org>
Date:	Tue, 8 Apr 2008 18:12:57 +0100
From:	Andy Whitcroft <apw@...dowen.org>
To:	Jan Engelhardt <jengelh@...putergmbh.de>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [patch] checkpatch: relax spacing and line length

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

Powered by blists - more mailing lists