[PATCH RFC] checkpatch: flag split arithmetic operations with CHECK

[PATCH RFC] checkpatch: flag split arithmetic operations with CHECK

[Nicholas Mc Guire]

Simple arithmetic operations should be on one line, if they can be fit,
rather than splitting at the operator. As this is not in the CodingStyle it 
is limited to --strict use of checkpatch.pl and emits a CHECK only.

Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
---

Patches by people like me splitting arithmetic operations, e.g.:
                       hdw->encoder_run_timer.expires = jiffies +
                                msecs_to_jiffies(TIME_MSEC_ENCODER_OK);
were flagged by Joe Perches but checkpatch.pl was not fussing, not even
with --strict. This extension should flag such lines as CHECK only, as 
this is not mandated by the CodingStyle and in some cases they are 
justified (e.g. kernel/sched/fair.c:760 and a few other examples in that
file)

Limitation: this is for + and - only still have to figure out some false
            positives for the * and /, % ($Arithmetic) case.

patch is against 4.0-rc2 (localversion-next is -next-20150505)

 scripts/checkpatch.pl |   13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 89b1df4..051ea99 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2577,6 +2577,19 @@ sub process {
 			    "Logical continuations should be on the previous line\n" . $hereprev);
 		}
 
+# check for + or - at the end of a line
+		if ($rawline =~ /^\+[^*]*(\+|-)$/) {
+			CHK("ARITHMETIC_CONTINUATIONS",
+			    "Arithmetic expressions should be on one line\n" . $hereprev);
+		}
+
+# check for + or - at beginning of a line but exclude ++var/--var
+		if (!($rawline =~ /^\+\s*(\+\+|--).*$/) &&
+		   $rawline =~ /^\+\s*(\+|-).*$/) {
+			CHK("ARITHMETIC_CONTINUATIONS",
+			    "Arithmetic expressions should be on one line\n" . $hereprev);
+		}
+
 # check multi-line statement indentation matches previous line
 		if ($^V && $^V ge 5.10.0 &&
 		    $prevline =~ /^\+([ \t]*)((?:$c90_Keywords(?:\s+if)\s*)|(?:$Declare\s*)?(?:$Ident|\(\s*\*\s*$Ident\s*\))\s*|$Ident\s*=\s*$Ident\s*)\(.*(\&\&|\|\||,)\s*$/) {
-- 
1.7.10.4

Re: [PATCH RFC] checkpatch: flag split arithmetic operations with CHECK

[Andy Whitcroft]

On Tue, May 05, 2015 at 10:53:36AM +0200, Nicholas Mc Guire wrote:
> Simple arithmetic operations should be on one line, if they can be fit,
> rather than splitting at the operator. As this is not in the CodingStyle it 
> is limited to --strict use of checkpatch.pl and emits a CHECK only.
> 
> Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
> ---
> 
> Patches by people like me splitting arithmetic operations, e.g.:
>                        hdw->encoder_run_timer.expires = jiffies +
>                                 msecs_to_jiffies(TIME_MSEC_ENCODER_OK);
> were flagged by Joe Perches but checkpatch.pl was not fussing, not even
> with --strict. This extension should flag such lines as CHECK only, as 
> this is not mandated by the CodingStyle and in some cases they are 
> justified (e.g. kernel/sched/fair.c:760 and a few other examples in that
> file)
> 
> Limitation: this is for + and - only still have to figure out some false
>             positives for the * and /, % ($Arithmetic) case.

I assume that these relate to being able to confirm the variant of the
operator, unary/binary etc.  If you look for "annotate_values", after
that is run there is additional information for each character of the
current line, tracking the type of the operator.  This is used when
determining spacing for + etc later as unary ones are typically tight
bound and binary ones spaced out.  You might find that useful, if that
is your issue.  And indeed you might find it useful for determining if
the +/- you find at line end is indeed unary.  Running with --debug
values=1 should dump out the variants information for those.

> 
> patch is against 4.0-rc2 (localversion-next is -next-20150505)
> 
>  scripts/checkpatch.pl |   13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 89b1df4..051ea99 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -2577,6 +2577,19 @@ sub process {
>  			    "Logical continuations should be on the previous line\n" . $hereprev);
>  		}
>  
> +# check for + or - at the end of a line
> +		if ($rawline =~ /^\+[^*]*(\+|-)$/) {
> +			CHK("ARITHMETIC_CONTINUATIONS",
> +			    "Arithmetic expressions should be on one line\n" . $hereprev);
> +		}
> +
> +# check for + or - at beginning of a line but exclude ++var/--var
> +		if (!($rawline =~ /^\+\s*(\+\+|--).*$/) &&
> +		   $rawline =~ /^\+\s*(\+|-).*$/) {
> +			CHK("ARITHMETIC_CONTINUATIONS",
> +			    "Arithmetic expressions should be on one line\n" . $hereprev);
> +		}
> +
>  # check multi-line statement indentation matches previous line
>  		if ($^V && $^V ge 5.10.0 &&
>  		    $prevline =~ /^\+([ \t]*)((?:$c90_Keywords(?:\s+if)\s*)|(?:$Declare\s*)?(?:$Ident|\(\s*\*\s*$Ident\s*\))\s*|$Ident\s*=\s*$Ident\s*)\(.*(\&\&|\|\||,)\s*$/) {
> -- 
> 1.7.10.4

-apw

Re: [PATCH RFC] checkpatch: flag split arithmetic operations with CHECK

[Nicholas Mc Guire]

On Tue, 05 May 2015, Andy Whitcroft wrote:

> On Tue, May 05, 2015 at 10:53:36AM +0200, Nicholas Mc Guire wrote:
> > Simple arithmetic operations should be on one line, if they can be fit,
> > rather than splitting at the operator. As this is not in the CodingStyle it 
> > is limited to --strict use of checkpatch.pl and emits a CHECK only.
> > 
> > Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
> > ---
> > 
> > Patches by people like me splitting arithmetic operations, e.g.:
> >                        hdw->encoder_run_timer.expires = jiffies +
> >                                 msecs_to_jiffies(TIME_MSEC_ENCODER_OK);
> > were flagged by Joe Perches but checkpatch.pl was not fussing, not even
> > with --strict. This extension should flag such lines as CHECK only, as 
> > this is not mandated by the CodingStyle and in some cases they are 
> > justified (e.g. kernel/sched/fair.c:760 and a few other examples in that
> > file)
> > 
> > Limitation: this is for + and - only still have to figure out some false
> >             positives for the * and /, % ($Arithmetic) case.
> 
> I assume that these relate to being able to confirm the variant of the
> operator, unary/binary etc.  If you look for "annotate_values", after
> that is run there is additional information for each character of the
> current line, tracking the type of the operator.  This is used when
> determining spacing for + etc later as unary ones are typically tight
> bound and binary ones spaced out.  You might find that useful, if that
> is your issue.  And indeed you might find it useful for determining if
> the +/- you find at line end is indeed unary.  Running with --debug
> values=1 should dump out the variants information for those.
>
thanks - looks like a useful (somewhat cryptic) starting point

15 > .  x = y - x
15 > EEEVVNNVVNNTTT
15 >  ________B____

will give it a try to generalize it for all basic binary operators.

thx!
hofrat 

Re: [PATCH RFC] checkpatch: flag split arithmetic operations with CHECK

[Joe Perches]

On Tue, 2015-05-05 at 16:08 +0200, Nicholas Mc Guire wrote:
> On Tue, 05 May 2015, Andy Whitcroft wrote:
> > On Tue, May 05, 2015 at 10:53:36AM +0200, Nicholas Mc Guire wrote:
> > > Simple arithmetic operations should be on one line, if they can be fit,
> > > rather than splitting at the operator. As this is not in the CodingStyle it 
> > > is limited to --strict use of checkpatch.pl and emits a CHECK only.
[]
> > I assume that these relate to being able to confirm the variant of the
> > operator, unary/binary etc.  If you look for "annotate_values", after
> > that is run there is additional information for each character of the
> > current line, tracking the type of the operator.  This is used when
> > determining spacing for + etc later as unary ones are typically tight
> > bound and binary ones spaced out.  You might find that useful, if that
> > is your issue.  And indeed you might find it useful for determining if
> > the +/- you find at line end is indeed unary.  Running with --debug
> > values=1 should dump out the variants information for those.
> >
> thanks - looks like a useful (somewhat cryptic) starting point
> 
> 15 > .  x = y - x
> 15 > EEEVVNNVVNNTTT
> 15 >  ________B____
> 
> will give it a try to generalize it for all basic binary operators.

Look at the code for "# Check operator spacing" around line 3600.
It's a pretty big block of ~300 lines of code, but this new test
should go in there.

Re: [PATCH RFC] checkpatch: flag split arithmetic operations with CHECK

[Nicholas Mc Guire]

On Tue, 05 May 2015, Joe Perches wrote:

> On Tue, 2015-05-05 at 16:08 +0200, Nicholas Mc Guire wrote:
> > On Tue, 05 May 2015, Andy Whitcroft wrote:
> > > On Tue, May 05, 2015 at 10:53:36AM +0200, Nicholas Mc Guire wrote:
> > > > Simple arithmetic operations should be on one line, if they can be fit,
> > > > rather than splitting at the operator. As this is not in the CodingStyle it 
> > > > is limited to --strict use of checkpatch.pl and emits a CHECK only.
> []
> > > I assume that these relate to being able to confirm the variant of the
> > > operator, unary/binary etc.  If you look for "annotate_values", after
> > > that is run there is additional information for each character of the
> > > current line, tracking the type of the operator.  This is used when
> > > determining spacing for + etc later as unary ones are typically tight
> > > bound and binary ones spaced out.  You might find that useful, if that
> > > is your issue.  And indeed you might find it useful for determining if
> > > the +/- you find at line end is indeed unary.  Running with --debug
> > > values=1 should dump out the variants information for those.
> > >
> > thanks - looks like a useful (somewhat cryptic) starting point
> > 
> > 15 > .  x = y - x
> > 15 > EEEVVNNVVNNTTT
> > 15 >  ________B____
> > 
> > will give it a try to generalize it for all basic binary operators.
> 
> Look at the code for "# Check operator spacing" around line 3600.
> It's a pretty big block of ~300 lines of code, but this new test
> should go in there.
>
will move it then once its working - checkpatch.pl is a bit hard to navigate
in for me - was not clear where it would fit in. will cleanup and repost.

thx!
hofrat