The 80-character limit is not a hard-and-fast rule, nor should it be
applied blindly by people running checkpatch and fixing its warnings.
Sometimes it's better to violate the 80-character "limit" in the name of
readability, and when it isn't, it's often better to refactor into a
function or otherwise restructure the code rather than just finding
increasingly awkward places to break lines.
Thus, change checkpatch's LONG_LINE warning to a --strict CHK instead.
Anyone wanting to use checkpatch to check for this can easily enough
enable --strict or turn on LONG_LINE explicitly, but it shouldn't be
part of the default warnings.
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 47016c3..ed16a68 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2017,8 +2017,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
--
1.8.4.rc3
On Mon, 2013-10-07 at 12:18 -0700, Josh Triplett wrote:
> The 80-character limit is not a hard-and-fast rule, nor should it be
> applied blindly by people running checkpatch and fixing its warnings.
> Sometimes it's better to violate the 80-character "limit" in the name of
> readability, and when it isn't, it's often better to refactor into a
> function or otherwise restructure the code rather than just finding
> increasingly awkward places to break lines.
>
> Thus, change checkpatch's LONG_LINE warning to a --strict CHK instead.
> Anyone wanting to use checkpatch to check for this can easily enough
> enable --strict or turn on LONG_LINE explicitly, but it shouldn't be
> part of the default warnings.
I don't agree with this.
CodingStyle says:
----------------------
The limit on the length of lines is 80 columns and this is a strongly
preferred limit.
----------------------
People should be encouraged to use 80 column lines and as well
should learn to ignore messages they don't agree with.
If people are using checkpatch prior to any scripted git am,
then just as easily they could add --ignore=LONG_LINE.
On Mon, Oct 07, 2013 at 12:28:26PM -0700, Joe Perches wrote:
> On Mon, 2013-10-07 at 12:18 -0700, Josh Triplett wrote:
> > The 80-character limit is not a hard-and-fast rule, nor should it be
> > applied blindly by people running checkpatch and fixing its warnings.
> > Sometimes it's better to violate the 80-character "limit" in the name of
> > readability, and when it isn't, it's often better to refactor into a
> > function or otherwise restructure the code rather than just finding
> > increasingly awkward places to break lines.
> >
> > Thus, change checkpatch's LONG_LINE warning to a --strict CHK instead.
> > Anyone wanting to use checkpatch to check for this can easily enough
> > enable --strict or turn on LONG_LINE explicitly, but it shouldn't be
> > part of the default warnings.
>
> I don't agree with this.
>
> CodingStyle says:
> ----------------------
> The limit on the length of lines is 80 columns and this is a strongly
> preferred limit.
> ----------------------
Which is the subject of much controversy and extensive discussion, and
the consensus on the list (including by many maintainers) frequently
differs from that.
> People should be encouraged to use 80 column lines and as well
> should learn to ignore messages they don't agree with.
I've seen far more examples of the 80-column limit making code less
readable rather than more. It's only really helpful when it forces code
restructuring, *not* when it just forces an arbitrary line break.
> If people are using checkpatch prior to any scripted git am,
> then just as easily they could add --ignore=LONG_LINE.
Which random folks running checkpatch on staging drivers and trying to
help don't necessarily know to do. The defaults should cater to the
primary use case, and the 80-column limit is not something to apply
blindly. It falls in the same category as some of the warnings the
kernel emits with W=2 or so: sometimes helpful, often noise.
- Josh Triplett
On Mon, 2013-10-07 at 12:34 -0700, Josh Triplett wrote:
> On Mon, Oct 07, 2013 at 12:28:26PM -0700, Joe Perches wrote:
> > On Mon, 2013-10-07 at 12:18 -0700, Josh Triplett wrote:
> > > The 80-character limit is not a hard-and-fast rule, nor should it be
> > > applied blindly by people running checkpatch and fixing its warnings.
> > > Sometimes it's better to violate the 80-character "limit" in the name of
> > > readability, and when it isn't, it's often better to refactor into a
> > > function or otherwise restructure the code rather than just finding
> > > increasingly awkward places to break lines.
> > >
> > > Thus, change checkpatch's LONG_LINE warning to a --strict CHK instead.
> > > Anyone wanting to use checkpatch to check for this can easily enough
> > > enable --strict or turn on LONG_LINE explicitly, but it shouldn't be
> > > part of the default warnings.
> >
> > I don't agree with this.
> >
> > CodingStyle says:
> > ----------------------
> > The limit on the length of lines is 80 columns and this is a strongly
> > preferred limit.
> > ----------------------
>
> Which is the subject of much controversy and extensive discussion, and
> the consensus on the list (including by many maintainers) frequently
> differs from that.
Been there, had that discussion.
https://lkml.org/lkml/2009/12/18/3
I'm not applying/acking this.
On Mon, Oct 7, 2013 at 9:18 PM, Josh Triplett <[email protected]> wrote:
> The 80-character limit is not a hard-and-fast rule, nor should it be
> applied blindly by people running checkpatch and fixing its warnings.
> Sometimes it's better to violate the 80-character "limit" in the name of
> readability, and when it isn't, it's often better to refactor into a
> function or otherwise restructure the code rather than just finding
> increasingly awkward places to break lines.
>
> Thus, change checkpatch's LONG_LINE warning to a --strict CHK instead.
> Anyone wanting to use checkpatch to check for this can easily enough
> enable --strict or turn on LONG_LINE explicitly, but it shouldn't be
> part of the default warnings.
>
> Signed-off-by: Josh Triplett <[email protected]>
As I'm not a huge fan of the 80-character limit,
Acked-by: Richard Weinberger <[email protected]>
> ---
>
> scripts/checkpatch.pl | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 47016c3..ed16a68 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -2017,8 +2017,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
> --
> 1.8.4.rc3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
Thanks,
//richard
On Mon, Oct 07, 2013 at 12:34:33PM -0700, Josh Triplett wrote:
> I've seen far more examples of the 80-column limit making code less
> readable rather than more. It's only really helpful when it forces code
> restructuring, *not* when it just forces an arbitrary line break.
So teach that piece of crap to complain about fucked-in-head line breaks like
ret_val =
leaf_shift_left(tb,
tb->
lnum
[0],
tb->
lbytes
-
1);
in addition to obscenely long lines (and yes, it is a real-world example).
The one and only point of such tools is to help locating the crappy code.
And that's the only sane criterion for evaluating new "stylistic rules" -
does that particular heuristic catch enough shitty places or not?
_Anything_ can be obfuscated to the point where warnings are not produced
anymore...
On Mon, 2013-10-07 at 22:23 +0100, Al Viro wrote:
> On Mon, Oct 07, 2013 at 12:34:33PM -0700, Josh Triplett wrote:
>
> > I've seen far more examples of the 80-column limit making code less
> > readable rather than more. It's only really helpful when it forces code
> > restructuring, *not* when it just forces an arbitrary line break.
>
> So teach that piece of crap to complain about fucked-in-head line breaks like
> ret_val =
> leaf_shift_left(tb,
> tb->
> lnum
> [0],
> tb->
> lbytes
> -
> 1);
> in addition to obscenely long lines (and yes, it is a real-world example).
Yup, that's pretty ugly.
It was autogenerated and introduced when Linus
rather arbitrarily lindented the reiserfs code.
btw: I think lindent should be removed from scripts/
http://lkml.org/lkml/2013/2/11/390
I believe reiserfs was originally set to a 132
column width maximum.
commit bd4c625c061c2a38568d0add3478f59172455159
Author: Linus Torvalds <[email protected]>
Date: Tue Jul 12 20:21:28 2005 -0700
reiserfs: run scripts/Lindent on reiserfs code
This was a pure indentation change, using:
scripts/Lindent fs/reiserfs/*.c include/linux/reiserfs_*.h
Josh Triplett <[email protected]> writes:
> The 80-character limit is not a hard-and-fast rule, nor should it be
> applied blindly by people running checkpatch and fixing its warnings.
> Sometimes it's better to violate the 80-character "limit" in the name of
> readability, and when it isn't, it's often better to refactor into a
> function or otherwise restructure the code rather than just finding
> increasingly awkward places to break lines.
Yes please!
-Andi
--
[email protected] -- Speaking for myself only
On Mon, 2013-10-07 at 14:33 -0700, Joe Perches wrote:
> On Mon, 2013-10-07 at 22:23 +0100, Al Viro wrote:
> > On Mon, Oct 07, 2013 at 12:34:33PM -0700, Josh Triplett wrote:
> >
> > > I've seen far more examples of the 80-column limit making code less
> > > readable rather than more. It's only really helpful when it forces code
> > > restructuring, *not* when it just forces an arbitrary line break.
> >
> > So teach that piece of crap to complain about fucked-in-head line breaks like
> > ret_val =
> > leaf_shift_left(tb,
> > tb->
> > lnum
> > [0],
> > tb->
> > lbytes
> > -
> > 1);
> > in addition to obscenely long lines (and yes, it is a real-world example).
btw: checkpatch does suggest refactoring when there are
6+ indent tabs with an if/while/for/do/switch statement.
so it already does bleat around there.
On Mon, Oct 07, 2013 at 12:38:20PM -0700, Joe Perches wrote:
> On Mon, 2013-10-07 at 12:34 -0700, Josh Triplett wrote:
> > On Mon, Oct 07, 2013 at 12:28:26PM -0700, Joe Perches wrote:
> > > On Mon, 2013-10-07 at 12:18 -0700, Josh Triplett wrote:
> > > > The 80-character limit is not a hard-and-fast rule, nor should it be
> > > > applied blindly by people running checkpatch and fixing its warnings.
> > > > Sometimes it's better to violate the 80-character "limit" in the name of
> > > > readability, and when it isn't, it's often better to refactor into a
> > > > function or otherwise restructure the code rather than just finding
> > > > increasingly awkward places to break lines.
> > > >
> > > > Thus, change checkpatch's LONG_LINE warning to a --strict CHK instead.
> > > > Anyone wanting to use checkpatch to check for this can easily enough
> > > > enable --strict or turn on LONG_LINE explicitly, but it shouldn't be
> > > > part of the default warnings.
> > >
> > > I don't agree with this.
> > >
> > > CodingStyle says:
> > > ----------------------
> > > The limit on the length of lines is 80 columns and this is a strongly
> > > preferred limit.
> > > ----------------------
> >
> > Which is the subject of much controversy and extensive discussion, and
> > the consensus on the list (including by many maintainers) frequently
> > differs from that.
>
> Been there, had that discussion.
> https://lkml.org/lkml/2009/12/18/3
I see many positive responses in that thread, from Linus and others, and
very little negativity (mostly quibbles about implementation, not about
the overall proposal). What was the problem?
In particular:
> I'll happily remove the checkpatch.pl limit entirely, and ask people to
> try to use common sense, though.
Between that and the positive responses in this thread, I'd love to see
your proposed patch revived.
- Josh Triplett
On Mon, Oct 07, 2013 at 12:18:43PM -0700, Josh Triplett wrote:
> The 80-character limit is not a hard-and-fast rule, nor should it be
> applied blindly by people running checkpatch and fixing its warnings.
> Sometimes it's better to violate the 80-character "limit" in the name of
> readability, and when it isn't, it's often better to refactor into a
> function or otherwise restructure the code rather than just finding
> increasingly awkward places to break lines.
>
> Thus, change checkpatch's LONG_LINE warning to a --strict CHK instead.
> Anyone wanting to use checkpatch to check for this can easily enough
> enable --strict or turn on LONG_LINE explicitly, but it shouldn't be
> part of the default warnings.
>
> Signed-off-by: Josh Triplett <[email protected]>
Acked-by: Greg Kroah-Hartman <[email protected]>