scripts/checkpatch.pl: Only emit LONG_LINE for --strict
PATCH 1/1 scripts/checkpatch.pl: Only emit LONG_LINE for --strict
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.
--- scripts/checkpatch.pl | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
scripts/checkpatch.pl +2 -2
@@ -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
Signature · 1 line
2.0.0
Joe Perches
maintainerQuoted 2 lines · Josh Triplett
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.
Quoted 11 lines · Joe Perches
Quoted 2 lines · Josh Triplett
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
Joe Perches
maintainerQuoted 7 lines · Josh Triplett
Quoted 6 lines · Joe Perches
Quoted 2 lines · Josh Triplett
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.
[]
Quoted 3 lines
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.
Quoted 2 lines
I'm hoping that Greg will chime in, as the maintainer of staging and the recipient of a huge number of checkpatch-motivated patches.
Quoted 8 lines · Joe Perches
Quoted 7 lines · Josh Triplett
Quoted 6 lines · Joe Perches
Quoted 2 lines · Josh Triplett
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.
Quoted 8 lines
[]
Quoted 3 lines
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
Joe Perches
maintainerQuoted 18 lines · Josh Triplett
Quoted 8 lines · Joe Perches
Quoted 7 lines · Josh Triplett
Quoted 6 lines · Joe Perches
Quoted 2 lines · Josh Triplett
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.Quoted 8 lines
[]
Quoted 3 lines
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.
Quoted 6 lines
Not arguing with that, but in this particular case the warning seems counterproductive to that goal, especially compared to the
DEEP_INDENTATIONwarning. 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...
Quoted 26 lines · Joe Perches
Quoted 18 lines · Josh Triplett
Quoted 8 lines · Joe Perches
Quoted 7 lines · Josh Triplett
Quoted 6 lines · Joe Perches
Quoted 2 lines · Josh Triplett
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.Quoted 8 lines
[]
Quoted 3 lines
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.
Quoted 6 lines
Not arguing with that, but in this particular case the warning seems counterproductive to that goal, especially compared to the
DEEP_INDENTATIONwarning. More subjective or "to taste" issues tend to have lower severity in checkpatch. And CodingStyle *already* says "unless exceeding 80 columns significantly increases readability"
Quoted 4 lines
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.
Quoted 3 lines
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.
Quoted 4 lines
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.
Quoted 3 lines
Multiple logical tests on a single line:
foo > bar && baz < quz && etc...
If any individual test takes up a significant number of columns, sure.
Quoted 9 lines
Arguments after column 80
1
1 2 3 4 5 6 7 8 9 0
1234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890result = 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.
Quoted 3 lines
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.
Quoted 1 line
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
Joe Perches
maintainerQuoted 3 lines · Josh Triplett
Now, arguably the four leading tabs on those lines suggest the need for some code refactoring; personally, I'd suggest changing
DEEP_INDENTATIONto flag 4+ tabs rather than 6+ tabs as it currently does.
There are _way too many_ 4+ tab indents for that to be sensible.
Quoted 1 line
"trigraphs"? Do you mean ternaries? Yeah,
Yup, those.
Quoted 3 lines
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.
Quoted 8 lines · Joe Perches
Quoted 3 lines · Josh Triplett
Now, arguably the four leading tabs on those lines suggest the need for some code refactoring; personally, I'd suggest changing
DEEP_INDENTATIONto flag 4+ tabs rather than 6+ tabs as it currently does.There are
_waytoomany_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
Quoted 13 lines · Josh Triplett
Quoted 11 lines · Joe Perches
Quoted 2 lines · Josh Triplett
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.
Quoted 6 lines
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
Quoted 18 lines · Greg KH
Quoted 17 lines · Josh Triplett
Quoted 11 lines · Joe Perches
Quoted 2 lines · Josh Triplett
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.
Quoted 6 lines
Quoted 2 lines
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
Quoted 2 lines · Josh Triplett
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.