Regardless of the long-standing debate over line width, checkpatch
should not warn about it by default. Too many users run checkpatch and
blindly follow its recommendation by splitting long lines, which almost
invariably results in worse code. On rare occasions, the line-width
limit encourages sensible refactoring of nested code into functions, but
more frequently it just results in painfully over-wrapped code. Turning
this warning off by default will ensure that people who take the time to
fix up checkpatch issues in drivers (especially staging drivers) don't
waste time submitting patches that uglify code to quiet checkpatch's
line-width limit.
The on-by-default DEEP_INDENTATION warning for lines indented more than
6 levels deep makes more sense as a default, to encourage people to
refactor, since it cannot be "fixed" by simply reformatting code.
Signed-off-by: Josh Triplett <[email protected]>
---
scripts/checkpatch.pl | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 010b18e..b2eb968 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2146,8 +2146,8 @@ sub process {
$line =~ /^\+\s*"[^"]*"\s*(?:\s*|,|\)\s*;)\s*$/) &&
$length > $max_line_length)
{
- WARN("LONG_LINE",
- "line over $max_line_length characters\n" . $herecurr);
+ CHK("LONG_LINE",
+ "line over $max_line_length characters\n" . $herecurr);
}
# Check for user-visible strings broken across lines, which breaks the ability
--
2.0.0
On Wed, 2014-06-25 at 08:46 -0700, Josh Triplett wrote:
> Regardless of the long-standing debate over line width, checkpatch
> should not warn about it by default.
I'm not getting involved here.
I don't care much one way or another.
I did submit a patch where I ignored 80
columns recently and I was told to try
again by the maintainer.
On Wed, Jun 25, 2014 at 05:05:07PM -0700, Joe Perches wrote:
> On Wed, 2014-06-25 at 08:46 -0700, Josh Triplett wrote:
> > Regardless of the long-standing debate over line width, checkpatch
> > should not warn about it by default.
>
> I'm not getting involved here.
>
> I don't care much one way or another.
>
> I did submit a patch where I ignored 80
> columns recently and I was told to try
> again by the maintainer.
I'm not asking you to get involved in the Great Line Length Debate;
that's why I didn't attempt to patch CodingStyle or similar. However, I
think it makes sense for *checkpatch* to stop emitting this warning.
I'm hoping that Greg will chime in, as the maintainer of staging and the
recipient of a huge number of checkpatch-motivated patches.
- Josh Triplett
On Wed, 2014-06-25 at 19:24 -0700, Josh Triplett wrote:
> On Wed, Jun 25, 2014 at 05:05:07PM -0700, Joe Perches wrote:
> > On Wed, 2014-06-25 at 08:46 -0700, Josh Triplett wrote:
> > > Regardless of the long-standing debate over line width, checkpatch
> > > should not warn about it by default.
> >
> > I'm not getting involved here.
> > I don't care much one way or another.
[]
> I'm not asking you to get involved in the Great Line Length Debate;
> that's why I didn't attempt to patch CodingStyle or similar. However, I
> think it makes sense for *checkpatch* to stop emitting this warning.
I think checkpatch should encourage people to write code in
a style that matches CodingStyle as well as the predominant
styles used in the rest of the kernel.
> I'm hoping that Greg will chime in, as the maintainer of staging and the
> recipient of a huge number of checkpatch-motivated patches.
On Wed, Jun 25, 2014 at 07:33:03PM -0700, Joe Perches wrote:
> On Wed, 2014-06-25 at 19:24 -0700, Josh Triplett wrote:
> > On Wed, Jun 25, 2014 at 05:05:07PM -0700, Joe Perches wrote:
> > > On Wed, 2014-06-25 at 08:46 -0700, Josh Triplett wrote:
> > > > Regardless of the long-standing debate over line width, checkpatch
> > > > should not warn about it by default.
> > >
> > > I'm not getting involved here.
> > > I don't care much one way or another.
> []
> > I'm not asking you to get involved in the Great Line Length Debate;
> > that's why I didn't attempt to patch CodingStyle or similar. However, I
> > think it makes sense for *checkpatch* to stop emitting this warning.
>
> I think checkpatch should encourage people to write code in
> a style that matches CodingStyle as well as the predominant
> styles used in the rest of the kernel.
Not arguing with that, but in this particular case the warning seems
counterproductive to that goal, especially compared to the
DEEP_INDENTATION warning. More subjective or "to taste" issues tend
to have lower severity in checkpatch. And CodingStyle *already* says
"unless exceeding 80 columns significantly increases
readability"
- Josh Triplett
On Wed, Jun 25, 2014 at 07:24:49PM -0700, Josh Triplett wrote:
> On Wed, Jun 25, 2014 at 05:05:07PM -0700, Joe Perches wrote:
> > On Wed, 2014-06-25 at 08:46 -0700, Josh Triplett wrote:
> > > Regardless of the long-standing debate over line width, checkpatch
> > > should not warn about it by default.
> >
> > I'm not getting involved here.
> >
> > I don't care much one way or another.
> >
> > I did submit a patch where I ignored 80
> > columns recently and I was told to try
> > again by the maintainer.
>
> I'm not asking you to get involved in the Great Line Length Debate;
> that's why I didn't attempt to patch CodingStyle or similar. However, I
> think it makes sense for *checkpatch* to stop emitting this warning.
>
> I'm hoping that Greg will chime in, as the maintainer of staging and the
> recipient of a huge number of checkpatch-motivated patches.
I have no problem with the existing checkpatch.pl tool and it calling
out 80 columns as a problem that needs to be fixed. So I don't like
this patch at all.
greg k-h
On Wed, 2014-06-25 at 20:44 -0700, Josh Triplett wrote:
> On Wed, Jun 25, 2014 at 07:33:03PM -0700, Joe Perches wrote:
> > On Wed, 2014-06-25 at 19:24 -0700, Josh Triplett wrote:
> > > On Wed, Jun 25, 2014 at 05:05:07PM -0700, Joe Perches wrote:
> > > > On Wed, 2014-06-25 at 08:46 -0700, Josh Triplett wrote:
> > > > > Regardless of the long-standing debate over line width, checkpatch
> > > > > should not warn about it by default.
> > > >
> > > > I'm not getting involved here.
> > > > I don't care much one way or another.
> > []
> > > I'm not asking you to get involved in the Great Line Length Debate;
> > > that's why I didn't attempt to patch CodingStyle or similar. However, I
> > > think it makes sense for *checkpatch* to stop emitting this warning.
> >
> > I think checkpatch should encourage people to write code in
> > a style that matches CodingStyle as well as the predominant
> > styles used in the rest of the kernel.
>
> Not arguing with that, but in this particular case the warning seems
> counterproductive to that goal, especially compared to the
> DEEP_INDENTATION warning. More subjective or "to taste" issues tend
> to have lower severity in checkpatch. And CodingStyle *already* says
> "unless exceeding 80 columns significantly increases
> readability"
Just some suggestions off the top of my head.
If the goal is to reduce long line lengths, then maybe
more warnings or --strict tests for things like:
long variable names: (pick a length)
some_long_variable_name_with_a_color
consecutive dereferences where a temporary might be better:
foo->bar[baz].member1 = 1;
foo->bar[baz].member2 = 2;
Multiple logical tests on a single line:
foo > bar && baz < quz && etc...
Arguments after column 80
1
1 2 3 4 5 6 7 8 9 0
1234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890
result = some_long_function(some_arg(arg1, arg2), another_arg(arg3, arg4), 4);
where a single argument of a short length before a
statement terminating ; may be ignored, but a longer
argument list may not.
Long trigraphs:
should be on multiple lines
Comments beyond 80 column
No idea what's right.
etc...
On Wed, Jun 25, 2014 at 11:59:59PM -0400, Greg KH wrote:
> On Wed, Jun 25, 2014 at 07:24:49PM -0700, Josh Triplett wrote:
> > On Wed, Jun 25, 2014 at 05:05:07PM -0700, Joe Perches wrote:
> > > On Wed, 2014-06-25 at 08:46 -0700, Josh Triplett wrote:
> > > > Regardless of the long-standing debate over line width, checkpatch
> > > > should not warn about it by default.
> > >
> > > I'm not getting involved here.
> > >
> > > I don't care much one way or another.
> > >
> > > I did submit a patch where I ignored 80
> > > columns recently and I was told to try
> > > again by the maintainer.
> >
> > I'm not asking you to get involved in the Great Line Length Debate;
> > that's why I didn't attempt to patch CodingStyle or similar. However, I
> > think it makes sense for *checkpatch* to stop emitting this warning.
> >
> > I'm hoping that Greg will chime in, as the maintainer of staging and the
> > recipient of a huge number of checkpatch-motivated patches.
>
> I have no problem with the existing checkpatch.pl tool and it calling
> out 80 columns as a problem that needs to be fixed. So I don't like
> this patch at all.
I'd like to stop seeing patches go by that produce heavily over-wrapped
code that becomes less readable; it's far easier to fix checkpatch than
to tell people to ignore its false positives.
How would you feel about a patch that flagged long lines with a warning
in patch mode, but not in file mode?
- Josh Triplett
On Wed, Jun 25, 2014 at 09:16:51PM -0700, Joe Perches wrote:
> On Wed, 2014-06-25 at 20:44 -0700, Josh Triplett wrote:
> > On Wed, Jun 25, 2014 at 07:33:03PM -0700, Joe Perches wrote:
> > > On Wed, 2014-06-25 at 19:24 -0700, Josh Triplett wrote:
> > > > On Wed, Jun 25, 2014 at 05:05:07PM -0700, Joe Perches wrote:
> > > > > On Wed, 2014-06-25 at 08:46 -0700, Josh Triplett wrote:
> > > > > > Regardless of the long-standing debate over line width, checkpatch
> > > > > > should not warn about it by default.
> > > > >
> > > > > I'm not getting involved here.
> > > > > I don't care much one way or another.
> > > []
> > > > I'm not asking you to get involved in the Great Line Length Debate;
> > > > that's why I didn't attempt to patch CodingStyle or similar. However, I
> > > > think it makes sense for *checkpatch* to stop emitting this warning.
> > >
> > > I think checkpatch should encourage people to write code in
> > > a style that matches CodingStyle as well as the predominant
> > > styles used in the rest of the kernel.
> >
> > Not arguing with that, but in this particular case the warning seems
> > counterproductive to that goal, especially compared to the
> > DEEP_INDENTATION warning. More subjective or "to taste" issues tend
> > to have lower severity in checkpatch. And CodingStyle *already* says
> > "unless exceeding 80 columns significantly increases
> > readability"
>
> Just some suggestions off the top of my head.
>
> If the goal is to reduce long line lengths, then maybe
> more warnings or --strict tests for things like:
The goal is to make code more readable; a length guideline can help with
that, but a hard limit does more harm than good. And notably, we don't
*have* a hard limit, we have a guideline; however, checkpatch routinely
gets treated as a hard requirement rather than a guideline, and as such
it should avoid tests that have a high false-positive rate, which
LONG_LINE does.
> long variable names: (pick a length)
>
> some_long_variable_name_with_a_color
To avoid encouraging people to disemvowel their variables. I'd suggest
flagging identifiers_with_too_many_words or IdentifiersWithTooManyWords
instead.
> consecutive dereferences where a temporary might be better:
>
> foo->bar[baz].member1 = 1;
> foo->bar[baz].member2 = 2;
That one seems better done with coccinelle, actually.
> Multiple logical tests on a single line:
>
> foo > bar && baz < quz && etc...
If any individual test takes up a significant number of columns, sure.
> Arguments after column 80
> 1
> 1 2 3 4 5 6 7 8 9 0
> 1234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890
> result = some_long_function(some_arg(arg1, arg2), another_arg(arg3, arg4), 4);
>
> where a single argument of a short length before a
> statement terminating ; may be ignored, but a longer
> argument list may not.
That one seems like one of the most common cases where the 80-column
rule can result in less readable code. On the one hand, in the case
above, something like this would usually (but not always) improve the
code:
result = some_long_function(
some_arg(arg1, arg2),
another_arg(arg1, arg2),
4);
On the other hand, breaking within the arguments of some_arg or
another_arg seems completely and utterly wrong, and breaking around a
binary operator within the argument list would likewise make it painful
to read. I've seen things like that in numerous patches. where a single
expression that would only take ~100ish characters gets broken across
half a dozen lines because it starts with a few tabs worth of
indentation and has moderate-length (but completely reasonable) names:
result = sensible_function_name(
sensible_variable_name,
something(smallish)
+ something_else,
variable_name
& ~SOME_BIT_NAME);
That seems far more readable if written as
result = sensible_function_name(
sensible_variable_name,
something(smallish) + something_else,
variable_name & ~SOME_BIT_NAME);
even though the last two lines extend past 80 characters.
Now, arguably the four leading tabs on those lines suggest the need for
some code refactoring; personally, I'd suggest changing DEEP_INDENTATION
to flag 4+ tabs rather than 6+ tabs as it currently does. That would
mirror CodingStyle, which says "The answer to that is that if you need
more than 3 levels of indentation, you're screwed anyway, and should fix
your program." To avoid flagging too much existing code that has >4
levels of indentation but short, simple code, I'd suggest specifically
matching lines that have >4 tabs *and* >80 columns, and flagging the
indentation as the cause of the problem rather than the >80 columns.
I'm going to try sending a patch that keeps the existing 80-column
warning, but changes the message and guidance based on the content of
the line.
> Long trigraphs:
>
> should be on multiple lines
"trigraphs"? Do you mean ternaries? Yeah, that's a good case where if
the three components of the ternary together are too long, that's a
natural splitting point that shouldn't make the code less readable.
> Comments beyond 80 column
Yeah, that one makes sense: if you have a comment to the right of a long
line, and the comment is what's making it long, it's easy enough to move
the comment before the line.
- Josh Triplett
On Wed, 2014-06-25 at 22:08 -0700, Josh Triplett wrote:
> Now, arguably the four leading tabs on those lines suggest the need for
> some code refactoring; personally, I'd suggest changing DEEP_INDENTATION
> to flag 4+ tabs rather than 6+ tabs as it currently does.
There are _way too many_ 4+ tab indents for that to
be sensible.
> "trigraphs"? Do you mean ternaries? Yeah,
Yup, those.
> Yeah, that one makes sense: if you have a comment to the right of a long
> line, and the comment is what's making it long, it's easy enough to move
> the comment before the line.
On Wed, Jun 25, 2014 at 10:29:30PM -0700, Joe Perches wrote:
> On Wed, 2014-06-25 at 22:08 -0700, Josh Triplett wrote:
>
> > Now, arguably the four leading tabs on those lines suggest the need for
> > some code refactoring; personally, I'd suggest changing DEEP_INDENTATION
> > to flag 4+ tabs rather than 6+ tabs as it currently does.
>
> There are _way too many_ 4+ tab indents for that to
> be sensible.
Yeah, a quick grep confirmed that. On the other hand, if *already*
flagging a long line, it makes sense to flag 4+ tabs as potentially
excessive nesting.
- Josh Triplett
On Wed, Jun 25, 2014 at 09:43:27PM -0700, Josh Triplett wrote:
> How would you feel about a patch that flagged long lines with a warning
> in patch mode, but not in file mode?
Just tell people to not send patches to just cleanup checkpath.pl
warnings in code they don't actually maintain or make large changes
to. If that BS went away a lot of peoples life would be a lot easier.