All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH RFC] checkpatch: flag split arithmetic operations with CHECK
@ 2015-05-05  8:53 Nicholas Mc Guire
  2015-05-05  9:41 ` Andy Whitcroft
  0 siblings, 1 reply; 5+ messages in thread
From: Nicholas Mc Guire @ 2015-05-05  8:53 UTC (permalink / raw)
  To: Andy Whitcroft; +Cc: Joe Perches, linux-kernel, 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


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH RFC] checkpatch: flag split arithmetic operations with CHECK
  2015-05-05  8:53 [PATCH RFC] checkpatch: flag split arithmetic operations with CHECK Nicholas Mc Guire
@ 2015-05-05  9:41 ` Andy Whitcroft
  2015-05-05 14:08   ` Nicholas Mc Guire
  0 siblings, 1 reply; 5+ messages in thread
From: Andy Whitcroft @ 2015-05-05  9:41 UTC (permalink / raw)
  To: Nicholas Mc Guire; +Cc: Joe Perches, linux-kernel

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH RFC] checkpatch: flag split arithmetic operations with CHECK
  2015-05-05  9:41 ` Andy Whitcroft
@ 2015-05-05 14:08   ` Nicholas Mc Guire
  2015-05-05 15:44     ` Joe Perches
  0 siblings, 1 reply; 5+ messages in thread
From: Nicholas Mc Guire @ 2015-05-05 14:08 UTC (permalink / raw)
  To: Andy Whitcroft; +Cc: Nicholas Mc Guire, Joe Perches, linux-kernel

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 

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH RFC] checkpatch: flag split arithmetic operations with CHECK
  2015-05-05 14:08   ` Nicholas Mc Guire
@ 2015-05-05 15:44     ` Joe Perches
  2015-05-05 18:40       ` Nicholas Mc Guire
  0 siblings, 1 reply; 5+ messages in thread
From: Joe Perches @ 2015-05-05 15:44 UTC (permalink / raw)
  To: Nicholas Mc Guire; +Cc: Andy Whitcroft, Nicholas Mc Guire, linux-kernel

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.



^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH RFC] checkpatch: flag split arithmetic operations with CHECK
  2015-05-05 15:44     ` Joe Perches
@ 2015-05-05 18:40       ` Nicholas Mc Guire
  0 siblings, 0 replies; 5+ messages in thread
From: Nicholas Mc Guire @ 2015-05-05 18:40 UTC (permalink / raw)
  To: Joe Perches; +Cc: Andy Whitcroft, Nicholas Mc Guire, linux-kernel

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 

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2015-05-05 18:41 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-05-05  8:53 [PATCH RFC] checkpatch: flag split arithmetic operations with CHECK Nicholas Mc Guire
2015-05-05  9:41 ` Andy Whitcroft
2015-05-05 14:08   ` Nicholas Mc Guire
2015-05-05 15:44     ` Joe Perches
2015-05-05 18:40       ` Nicholas Mc Guire

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.