[PATCH] scripts/checkpatch.pl: Change long line warning to 105 chars

[PATCH] scripts/checkpatch.pl: Change long line warning to 105 chars

[Joe Perches]

Made the length test a variable for easier bike shedding.

Also add --strict tests for longer than 80 chars and more
than 6 leading tabs

Signed-off-by: Joe Perches <joe@perches.com>
---
 scripts/checkpatch.pl |   24 ++++++++++++++++++------
 1 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index a4d7434..2c5f30c 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -30,6 +30,10 @@ my $root;
 my %debug;
 my $help = 0;
 
+# Code style warnings
+my $style_long_line = 105;
+my $style_max_leading_tabs = 6;
+
 sub help {
 	my ($exitcode) = @_;
 
@@ -1385,13 +1389,21 @@ sub process {
 # check we are in a valid source file if not then ignore this hunk
 		next if ($realfile !~ /\.(h|c|s|S|pl|sh)$/);
 
-#80 column limit
-		if ($line =~ /^\+/ && $prevrawline !~ /\/\*\*/ &&
+#Line too long
+		if ($line =~ /^\+/ &&
+		    $prevrawline !~ /\/\*\*/ &&
 		    $rawline !~ /^.\s*\*\s*\@$Ident\s/ &&
-		    $line !~ /^\+\s*$logFunctions\s*\(\s*(?:KERN_\S+\s*)?"[X\t]*"\s*(?:,|\)\s*;)\s*$/ &&
-		    $length > 80)
-		{
-			WARN("line over 80 characters\n" . $herecurr);
+		    $line !~ /^\+\s*$logFunctions\s*\(\s*(?:KERN_\S+\s*)?"[X\t]*"\s*(?:,|\)\s*;)\s*$/) {
+			if ($length > $style_long_line) {
+				WARN("line over $style_long_line characters\n" . $herecurr);
+			} elsif ($length > 80) {
+				CHK("line over 80 characters\n" . $herecurr);
+			}
+		}
+
+#too many leading tabs - deep leading indent
+		if ($line =~ /^\+\t{$style_max_leading_tabs,}(?!(.*,$|.*\);$))/) {
+			CHK("Too many leading tabs.  Consider restructuring code\n" . $herecurr);
 		}
 
 # check for spaces before a quoted newline

[PATCH] scripts/checkpatch.pl: Change long line warning to 105 chars

[Joe Perches]

Hello Linus.

Could you pick up this patch directly please?

Andrew Morton doesn't want to as he believes using >80
column lines not nice, and Andy Whitcroft doesn't seem
to have much time to work on checkpatch these days.

Either this or perhaps just remove the >80 column test
altogether.

Thanks

-------------------------------------

Made the length test a variable for easier bike shedding.

Also add --strict tests for lines longer than 80 chars and
more than 6 leading tabs.

Signed-off-by: Joe Perches <joe@perches.com>
---
 scripts/checkpatch.pl |   24 ++++++++++++++++++------
 1 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index a4d7434..2c5f30c 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -30,6 +30,10 @@ my $root;
 my %debug;
 my $help = 0;
 
+# Code style warnings
+my $style_long_line = 105;
+my $style_max_leading_tabs = 6;
+
 sub help {
 	my ($exitcode) = @_;
 
@@ -1385,13 +1389,21 @@ sub process {
 # check we are in a valid source file if not then ignore this hunk
 		next if ($realfile !~ /\.(h|c|s|S|pl|sh)$/);
 
-#80 column limit
-		if ($line =~ /^\+/ && $prevrawline !~ /\/\*\*/ &&
+#Line too long
+		if ($line =~ /^\+/ &&
+		    $prevrawline !~ /\/\*\*/ &&
 		    $rawline !~ /^.\s*\*\s*\@$Ident\s/ &&
-		    $line !~ /^\+\s*$logFunctions\s*\(\s*(?:KERN_\S+\s*)?"[X\t]*"\s*(?:,|\)\s*;)\s*$/ &&
-		    $length > 80)
-		{
-			WARN("line over 80 characters\n" . $herecurr);
+		    $line !~ /^\+\s*$logFunctions\s*\(\s*(?:KERN_\S+\s*)?"[X\t]*"\s*(?:,|\)\s*;)\s*$/) {
+			if ($length > $style_long_line) {
+				WARN("line over $style_long_line characters\n" . $herecurr);
+			} elsif ($length > 80) {
+				CHK("line over 80 characters\n" . $herecurr);
+			}
+		}
+
+#too many leading tabs - deep leading indent
+		if ($line =~ /^\+\t{$style_max_leading_tabs,}(?!(.*,$|.*\);$))/) {
+			CHK("Too many leading tabs.  Consider restructuring code\n" . $herecurr);
 		}
 
 # check for spaces before a quoted newline

[PATCH] Drop 80-character limit in checkpatch.pl

[Mikulas Patocka]

Drop 80-character limit in checkpatch.pl

Serious issues:
===============

(1) The code is hard to edit with common text editors
-----------------------------------------------------

For example, let's have this function
struct some_structure *some_function(struct susystem *s, struct s_buffer *b, 
				     unsigned some_number, unsigned some_flags,
				     unsigned long *extra_return_value)

Now, try to make this function static. Or try to add/remove/change some
argument. You end up manually realigning all the arguments. The same problem
happens when editing longer expressions.

This is the most annoying problem with imposed text alignment. If there were
no alignment, you just type "I static SPACE ESC" in vi and get it there
(similarly HOME static SPACE in notepad). If you have to maintain alignment,
the number of keystokes becomes much higher:

			(insert the keyword)
I static SPACE ESC
			(split the second argument)
f , l s ENTER
			(align the second argument)
TAB TAB TAB TAB TAB SPACE SPACE SPACE SPACE ESC
		(see if the third argument fits --- it doesn't, revert it)
J u
			(align the third argument)
j ^ XXXXXX i TAB TAB SPACE SPACE SPACE SPACE ESC
			(split the fourth argument --- it is aligned well)
f , l s ENTER ESC
			(align the fifth argument)
j ^ XXXXXX i TAB TAB SPACE SPACE SPACE SPACE ESC

There are shorter possible sequences using vi keystrokes but the person doing
the editing doesn't always find the optimal way quickly enough, so the actual
editing will look somehow like this.

For emacs, there is a script in CodingStyle to make it conform to the Linux
kernel coding style, but the script only changes tabbing, it does absolutely
nothing with respect to the line width --- with the script loaded, emacs still
doesn't realign the arguments if you add a "static" keyword there.

[ I'm wondering, which editor does the person who invented and supports this
line-wrapping actually use. I suppose none :-) --- it looks like that person
mostly reads the code and tries to make it look aesthetical. ]

(2) Grepping the code is unreliable
-----------------------------------

For example, you see a message "device: some error happened" in syslog.
Now, you grep the kernel with a command (grep -r "some error happened" .) to
find out where it came from. You miss it if the code is formatted like this:
				if (some_error_happened) {
					printk(KERN_ERR "%s: some error "
						"happened", dev->name);
				}
In this case, you usually narrow the search to "error happened" and "some error"
and eventually find it.
Now, the real problem comes, when there are two instance of "some error
happened" message, one is wrapped and the other is not. Here, grep misleads you
to the non-wrapped instance and you are never going to find out that there is
another place where this error could be reported.

Similar grepping problems also exist (less frequently) when grepping the code
--- i.e. Where is the function "bla" called with argument BLA_SOME_FLAG?).
Grep and regular expressions give unreliable results because of 80-line
wrapping.

Less serious issues:
====================

(3) Wrapping wastes space on wide-screen displays
-------------------------------------------------

Obvious. The screen area past column 80 is not used at all.

(4) Wrapping wastes space on small 80x25 display
------------------------------------------------

It is less obvious, but let's see how artifical 80-column wrapping wastes space
on a real 80-column display.

With wrapping: 4 lines:
				struc = some_function(s, buffer, variable +
					get_something(123), FLAG_ONE |
					FLAG_TWO | FLAG_THREE | FLAG_FOUR,
					&extra_ret);

Without wrapping (note that the editor/viewer wraps it automatically at
column 80): 2 lines:
				struc = some_function(sub, buffer, variable + ge
t_something(123), FLAG_ONE | FLAG_TWO | FLAG_THREE | FLAG_FOUR, &extra_ret);

(5) Wrapping makes long expressions harder to understand
--------------------------------------------------------

If I have a complex expression, I do not try to wrap it at predefined
80-column boundaries, but at logical boundaries within the expression to make
it more readable (the brain can't find matching parentheses fast, so we can
help it by aligning the code according to topmost terms in the expression).

Example:
				if (unlikely(call_some_function(s, value) != RET
_SUCCESS) ||
				    (var_1 == prev_var_1 && var_2 == prev_var_2)
 ||
				    flags & (FLAG_1 | FLAG_2) ||
				    some_other_condition) {
				}

Now, if we impose 80-column limit, we get this. One may argue that is looks
aesthetically better, but it is also less intelligible than the previous
version:
				if (unlikely(call_some_function(s, value) !=
				    RET_SUCCESS) || (var_1 == prev_var_1 &&
				    var_2 == prev_var_2) || flags & (FLAG_1 |
				    FLAG_2) || some_other_condition) {
				}

Counter-arguments:
==================

Some people say that 80-column wrapping is imposed so that kernel code is
readable and editable in 80-column text mode.
(for example http://lkml.indiana.edu/hypermail/linux/kernel/0807.2/0010.html)

I personally use 80-column text mode a lot of time, yet I find the artifical
wrapping annoying. The key point is that the editor and viewer wrap the text
automatically at column 80, so there is never any text invisible and there is
no need to wrap it manually.

vi, less, mcview, emacs, grep, pine (*), mutt --- all these will automatically
skip to the next line if some line is longer than terminal width.

(*) pine does this auto-wrapping only in viewer mode, but no one writes code in
pine anyway.

If you rely on this auto-wrapping, you get split words (such as call_some_functi
on()), but I find these split words less annoying than having to manually
realign lines with tabs and spaces each time I edit something.

---

If this 80-column wrapping was only taken as a guidance --- use it or not, as
you like --- there would be little problem with it, because this manual
realigning doesn't annoy anyone but the person who writes the code.

But some maintainers take output of the script checkpatch.pl dogmatically,
requiring that every new work must pass the script without a warning. This is
counterproductive --- if I write a driver and I will be doing most maintenance
work on it in the future, it is viable that the driver is formatted in such
a way that is best editable for me, not for anyone else. And as shown in example
(1), this 80-column requirement makes even simple changes much harder.

So: I am submitting this patch for the checkpatch.pl script.


Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

---
 scripts/checkpatch.pl |    9 ---------
 1 file changed, 9 deletions(-)

Index: linux-2.6.32-devel/scripts/checkpatch.pl
===================================================================
--- linux-2.6.32-devel.orig/scripts/checkpatch.pl	2009-12-07 19:57:31.000000000 +0100
+++ linux-2.6.32-devel/scripts/checkpatch.pl	2009-12-07 19:57:44.000000000 +0100
@@ -1374,15 +1374,6 @@ sub process {
 # check we are in a valid source file if not then ignore this hunk
 		next if ($realfile !~ /\.(h|c|s|S|pl|sh)$/);
 
-#80 column limit
-		if ($line =~ /^\+/ && $prevrawline !~ /\/\*\*/ &&
-		    $rawline !~ /^.\s*\*\s*\@$Ident\s/ &&
-		    $line !~ /^\+\s*printk\s*\(\s*(?:KERN_\S+\s*)?"[X\t]*"\s*(?:,|\)\s*;)\s*$/ &&
-		    $length > 80)
-		{
-			WARN("line over 80 characters\n" . $herecurr);
-		}
-
 # check for adding lines without a newline.
 		if ($line =~ /^\+/ && defined $lines[$linenr] && $lines[$linenr] =~ /^\\ No newline at end of file/) {
 			WARN("adding a line without newline at end of file\n" . $herecurr);

Re: [PATCH] Drop 80-character limit in checkpatch.pl

[Paul Mundt]

On Tue, Dec 15, 2009 at 04:57:49PM -0500, Mikulas Patocka wrote:
> (5) Wrapping makes long expressions harder to understand
> --------------------------------------------------------
> 
> If I have a complex expression, I do not try to wrap it at predefined
> 80-column boundaries, but at logical boundaries within the expression to make
> it more readable (the brain can't find matching parentheses fast, so we can
> help it by aligning the code according to topmost terms in the expression).
> 
> Example:
> 				if (unlikely(call_some_function(s, value) != RET
> _SUCCESS) ||
> 				    (var_1 == prev_var_1 && var_2 == prev_var_2)
>  ||
> 				    flags & (FLAG_1 | FLAG_2) ||
> 				    some_other_condition) {
> 				}
> 
> Now, if we impose 80-column limit, we get this. One may argue that is looks
> aesthetically better, but it is also less intelligible than the previous
> version:
> 				if (unlikely(call_some_function(s, value) !=
> 				    RET_SUCCESS) || (var_1 == prev_var_1 &&
> 				    var_2 == prev_var_2) || flags & (FLAG_1 |
> 				    FLAG_2) || some_other_condition) {
> 				}
> 
For starters, this is just crap. If you're writing code like this, then
line wrapping is really the least of your concerns. Take your function
return value and assign it to a variable before testing it in unlikely()
as per existing conventions and most of this goes away in this example.
If you're testing an absurd amount of conditions in a single block with
needlessly verbose variable names, then yes, you will go over 80
characters. Consequently, your "clean" example doesn't look much better
than your purposely obfuscated one.

The 80 character limit isn't a hard limit, but it's still an excellent
guideline largely because it stops people from writing code like in your
above example. Some amount of common sense is necessary, and most
people in a position of applying patches can work out when to ignore
checkpatch and when not to. The exception that you also noted is that
some people run checkpatch on inbound patches before applying them, so
having a flag to ignore the line size (or at least make it non-fatal) is
probably not the worst idea ever -- although I disagree with killing off
any notice about the line size completely.

Aiming to keep things around 80 characters has served us well over the
years, so I don't quite agree with suddenly adopting an anything goes
policy in checkpatch. People that don't care about the limit can disable
it in their scripts while folks running their patches through prior to
list submission are still better off being reminded that they should keep
things reasonably close. One only has to take a look through some of the
staging/ drivers pre-cleanup efforts for examples that clearly benefit
from triggering these warnings rather than implicitly supporting insane
tab depths.

Re: [PATCH] Drop 80-character limit in checkpatch.pl

[Krzysztof Halasa]

Paul Mundt <lethal@linux-sh.org> writes:

> For starters, this is just crap. If you're writing code like this, then
> line wrapping is really the least of your concerns. Take your function
> return value and assign it to a variable before testing it in unlikely()
> as per existing conventions and most of this goes away in this example.
> If you're testing an absurd amount of conditions in a single block with
> needlessly verbose variable names, then yes, you will go over 80
> characters. Consequently, your "clean" example doesn't look much better
> than your purposely obfuscated one.

Then perhaps checkpatch should warn about too complex expressions
instead? Line length is rather weak indication of complexity.
The typical problem being printk() with the printed info approaching
80 chars (but there are others, of course).

BTW I remember we have already agreed that the 80-chars limit is a bad
thing, though apparently nobody bothered to fix the docs and checkpatch.

> The 80 character limit isn't a hard limit, but it's still an excellent
> guideline largely because it stops people from writing code like in your
> above example.

What is more important is it stops people from writing a perfectly
readable code, forcing them to make the code (much) worse.

> Some amount of common sense is necessary, and most
> people in a position of applying patches can work out when to ignore
> checkpatch and when not to.

Precisely - we need common sense instead of artificial limits which are
almost unrelated to the actual problem.

> Aiming to keep things around 80 characters has served us well over the
> years,

I don't think so. I think it made more harm than good. Some people
routinely ignore this "!!!!! ERROR !!!!!" but a large part of developers
make the code (sometimes much) worse for this very reason.
Fortunately the patches are reviewed.

> One only has to take a look through some of the
> staging/ drivers pre-cleanup efforts for examples that clearly benefit
> from triggering these warnings rather than implicitly supporting insane
> tab depths.

Checkpatch doesn't check for sane/insane tab depths. It's not that
trivial. Unfortunately, checking line length is a very poor substitute.

Yes, checkpatch probably could warn about having many long lines (though
I wouldn't mention the "80" number). But it should be clear it's not
about the length but about (possible) complexity.
-- 
Krzysztof Halasa

Re: [PATCH] Drop 80-character limit in checkpatch.pl

[Mikulas Patocka]

On Thu, 17 Dec 2009, Krzysztof Halasa wrote:

> Paul Mundt <lethal@linux-sh.org> writes:
> 
> > For starters, this is just crap. If you're writing code like this, then
> > line wrapping is really the least of your concerns. Take your function
> > return value and assign it to a variable before testing it in unlikely()
> > as per existing conventions and most of this goes away in this example.
> > If you're testing an absurd amount of conditions in a single block with
> > needlessly verbose variable names, then yes, you will go over 80
> > characters. Consequently, your "clean" example doesn't look much better
> > than your purposely obfuscated one.
> 
> Then perhaps checkpatch should warn about too complex expressions
> instead? Line length is rather weak indication of complexity.
> The typical problem being printk() with the printed info approaching
> 80 chars (but there are others, of course).

No. If someone wrote an expression too long to be understood, he will not 
understand it when maintaining his code, he burns himself and he will 
eventually learn to not write such long expressions. Or ... if he 
understands it, there is no problem with it.

There is no need to make a script for it. The script isn't so smart to 
tell what is understandable and what is nto.


All these coding-style discussions are just nit-picking. If you move the 
code up or down a few lines, it won't help anything.

The real problems of hard-to-understand code are totally different than 
"is this line too long" or "is this expression too complex".


Examples (what I personally found hard to understand):

* excessive inheritance in C++ --- if you have object->method(params), it 
is impossible to determine what is really called if the person is using 
inheritance trees deeper than two levels.

* excessively deep calls --- for example, python's module loader is 
written partially in python and partially in C and it is impossible to 
determine what is the control flow between the parts (see also the 
previous problem about inheritance). In Linux, an example is waking a 
process waiting on a bit, that calls 8 nested functions.

* excessive abstractions --- find that these two drivers have some common 
part and blast an abstraction layer with method tables in an effort to 
share these common parts. I have seen when two drivers were merged this 
way, the resulting driver was as big as those two separate drivers. What 
the sharing saved, the abstraction layer added.

* repeating the same logic again and again and not making it a macro or a 
function --- it's not seen in Linux kernel, but in other project I have 
seen the code for allocating and initializing a structure being repeated 
38 times. It's not hard to read, it's hard to modify. Linux community 
tends to be obsessive the opposite way (which leads to problems too, if 
done excessively), see the previous paragraph.

* code diffuse --- one thing at different places. An example is the Linux 
cdrom stack (ide-cd, sr, drivers/cdrom).

* constructing function names by concatenating macro arguments --- i.e. 
try to find with grep, where is "outb_p" defined.


--- and rules for code formatting fix really nothing.

Mikulas

Re: [PATCH] Drop 80-character limit in checkpatch.pl

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 865 bytes --]

On Thu, 17 Dec 2009 18:29:25 EST, Mikulas Patocka said:

> There is no need to make a script for it. The script isn't so smart to 
> tell what is understandable and what is nto.

Yeah, but I respectfully submit that if the regexp '^\t{6}' matches a non-
continuation line, it's probably in its rights to whinge. 

grep -r -P '^\t{7}(?!(.*,$|.*\);$))' . | more

produces a whole lot of "this can't end well" output. My favorite so far:

fs/reisersfs/do_balan.c, lines 460-477 (note: 3 leading tabs elided)

				leaf_paste_entries(&bi,
						   n +
						   item_pos
						   -
						   ret_val,
						   l_pos_in_item,
						   1,
						   (struct
						    reiserfs_de_head
						    *)
						   body,
						   body
						   +
						   DEH_SIZE,
						   tb->
						   insert_size
						   [0]
				    );

Yes, that used to be 24 more columns to the right.  Gaak.


[-- Attachment #2: Type: application/pgp-signature, Size: 227 bytes --]

[PATCH] scripts/checkpatch.pl: Change long line warning to 105 chars

[Joe Perches]

On Thu, 2009-12-17 at 23:29 -0500, Valdis.Kletnieks@vt.edu wrote:
> Yeah, but I respectfully submit that if the regexp '^\t{6}' matches a non-
> continuation line, it's probably in its rights to whinge. 
> 	grep -r -P '^\t{7:}(?!(.*,$|.*\);$))' . | more
> produces a whole lot of "this can't end well" output.

I think this is a good test to add to checkpatch.

Add 105 character long line WARN test
Add 80 character long line STRICT test
Add 6+ leading indent tabs test, consider restructuring

Signed-off-by: Joe Perches <joe@perches.com>

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index bc4114f..c358251 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1374,13 +1374,20 @@ sub process {
 # check we are in a valid source file if not then ignore this hunk
 		next if ($realfile !~ /\.(h|c|s|S|pl|sh)$/);
 
-#80 column limit
+#Line too long
 		if ($line =~ /^\+/ && $prevrawline !~ /\/\*\*/ &&
 		    $rawline !~ /^.\s*\*\s*\@$Ident\s/ &&
-		    $line !~ /^\+\s*printk\s*\(\s*(?:KERN_\S+\s*)?"[X\t]*"\s*(?:,|\)\s*;)\s*$/ &&
-		    $length > 80)
-		{
-			WARN("line over 80 characters\n" . $herecurr);
+		    $line !~ /^\+\s*printk\s*\(\s*(?:KERN_\S+\s*)?"[X\t]*"\s*(?:,|\)\s*;)\s*$/) {
+			if ($length > 105) {
+				WARN("line over 105 characters\n" . $herecurr);
+			} elsif ($length > 80) {
+				CHK("line over 80 characters\n" . $herecurr);
+			}
+		}
+
+#too many leading tabs - deep leading indent
+		if ($line =~ /^\+	{6,}(?!(.*,$|.*\);$))/) {
+			CHK("Too many leading tabs.  Consider restructuring code\n" . $herecurr);
 		}
 
 # check for adding lines without a newline.

Re: [PATCH] scripts/checkpatch.pl: Change long line warning to 105 chars

[Paul Mundt]

On Thu, Dec 17, 2009 at 09:12:24PM -0800, Joe Perches wrote:
> On Thu, 2009-12-17 at 23:29 -0500, Valdis.Kletnieks@vt.edu wrote:
> > Yeah, but I respectfully submit that if the regexp '^\t{6}' matches a non-
> > continuation line, it's probably in its rights to whinge. 
> > 	grep -r -P '^\t{7:}(?!(.*,$|.*\);$))' . | more
> > produces a whole lot of "this can't end well" output.
> 
> I think this is a good test to add to checkpatch.
> 
> Add 105 character long line WARN test
> Add 80 character long line STRICT test
> Add 6+ leading indent tabs test, consider restructuring
> 
This looks like a reasonable compromise.

Re: [PATCH] scripts/checkpatch.pl: Change long line warning to 105 chars

[Linus Torvalds]

On Fri, 18 Dec 2009, Paul Mundt wrote:

> On Thu, Dec 17, 2009 at 09:12:24PM -0800, Joe Perches wrote:
> > On Thu, 2009-12-17 at 23:29 -0500, Valdis.Kletnieks@vt.edu wrote:
> > > Yeah, but I respectfully submit that if the regexp '^\t{6}' matches a non-
> > > continuation line, it's probably in its rights to whinge. 
> > > 	grep -r -P '^\t{7:}(?!(.*,$|.*\);$))' . | more
> > > produces a whole lot of "this can't end well" output.
> > 
> > I think this is a good test to add to checkpatch.
> > 
> > Add 105 character long line WARN test
> > Add 80 character long line STRICT test
> > Add 6+ leading indent tabs test, consider restructuring
> > 
> This looks like a reasonable compromise.

I like this. Except I think the indent test should count spaces too some 
way. Or do we already warn about excessive space that should be tabs?

		Linus

Re: [PATCH] scripts/checkpatch.pl: Change long line warning to 105 chars

[Joe Perches]

On Fri, 2009-12-18 at 09:43 -0800, Linus Torvalds wrote:
> I like this. Except I think the indent test should count spaces too some 
> way. Or do we already warn about excessive space that should be tabs?

Andy's the checkpatch guy to verify this but:

There's a separate test for spaces that should be tabs.

If you had a 64 space indent and ran the silly script,
it'd complain about the spaces.

If you converted the spaces to tabs and ran the silly
script again with --strict, it'd complain about the
indent depth.

Re: [PATCH] scripts/checkpatch.pl: Change long line warning to 105 chars

[Andi Kleen]

Linus Torvalds <torvalds@linux-foundation.org> writes:

>
> I like this. Except I think the indent test should count spaces too some 
> way. Or do we already warn about excessive space that should be tabs?

checkpatch warns about those.

(it triggers for me all the time because those happen when you 
copy patch reject blocks from hand and remove the leading character
without removing the space afterwards)

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only.

Re: [PATCH] scripts/checkpatch.pl: Change long line warning to 105 chars

[Krzysztof Halasa]

Joe Perches <joe@perches.com> writes:

> I think this is a good test to add to checkpatch.
>
> Add 105 character long line WARN test

Not sure if that's long enough (maybe it is).
132 looks like the next "natural" number.
The problematic case like printk(KERN_DEBUG "%s:%s:%s", var ? "yes" :
"no" and so on. If we split the code like that then better use 132 than
105 - even with 132 chars we may need to split, remember it's the printk
output which has to fit in 80 chars, or maybe usually has.

The main problem is checkpatch can't estimate "complexity". Fortunately
the reviewers can.

> Add 80 character long line STRICT test

Not sure what do you mean.

> Add 6+ leading indent tabs test, consider restructuring

This makes perfect sense, at least until shown otherwise (though being
a warning instead of an error means it's ok to trigger in perfectly
valid code).
-- 
Krzysztof Halasa

Re: [PATCH] scripts/checkpatch.pl: Change long line warning to 105 chars

[Joe Perches]

On Fri, 2009-12-18 at 15:37 +0100, Krzysztof Halasa wrote:
> > Add 80 character long line STRICT test
> Not sure what do you mean.

CHK messages are only printed when --strict is on the command line

> This makes perfect sense, at least until shown otherwise (though being
> a warning instead of an error means it's ok to trigger in perfectly
> valid code).

It's a CHK, only printed with --strict.