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 <[email protected]>
---
V2: re-use the debug type detection as proposed by Andy Whitcroft
to distinguish the unary * from the binary * - thanks!
This now flags arithmetic operations split across lines indicated by
either a leading or final binary operator.
This extension should flag such lines as CHECK only, as this is not
mandated by the CodingStyle and in some cases they seem justified
(e.g. kernel/sched/fair.c:760 and a few other examples in that file)
TODO: Move to "# Check operator spacing" section as Joe Perches
requested - simply got completely lost in that section...
Also not clear if it really is the right place to put it -
spacing issues and split operators are two quite different
problems.
Question: There are many (518) "lines over 80 char" warnings but that
seems to be accepted in checkpatch.pl - should those be
wrapped ?
The 4 rules are left separated for review - most of this could be
joined but before doing that a review of the individual regex would
be good.
Test-case:
scripts/checkpatch.pl --strict -f kernel/*.c | grep -A 3 -e "Arithmetic"
(and a few other directories) - there seem to be no false positive there
Patch is against 4.1-rc2 (localversion-next is -next-20150508)
scripts/checkpatch.pl | 52 +++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 52 insertions(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 89b1df4..a2e3d26 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2577,6 +2577,58 @@ sub process {
"Logical continuations should be on the previous line\n" . $hereprev);
}
+# check for + or - at the end of a line but ignore --/++
+ if (!($rawline =~ /^\+.*(\+\+|--).*$/) &&
+ $rawline =~ /^\+.*(\+|-)$/) {
+ my $opline = $line; $opline =~ s/^./ /;
+ my ($curr_values, $types) = annotate_values($opline . "\n", $prev_values);
+ # its type and the end 'B' -> binary * -> fuss
+ if($types =~ m/^.*B$/) {
+ 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*(\+|-).*$/) {
+ # need to check the type here
+ my $opline = $line; $opline =~ s/^./ /;
+ my ($curr_values, $types) = annotate_values($opline . "\n", $prev_values);
+ # first is type 'B' -> binary * -> fuss
+ if($types =~ m/(^_*)(B)/) {
+ CHK("ARITHMETIC_CONTINUATIONS",
+ "Arithmetic expressions should be on one line\n" . $hereprev);
+ }
+ }
+
+# check for /,% and * at end of a line
+ if (!($rawline =~ /^\+.*\*\/$/) &&
+ $rawline =~ /^\+.*(\*|\/|%)$/) {
+ # need to check the type here
+ my $opline = $line; $opline =~ s/^./ /;
+ my ($curr_values, $types) = annotate_values($opline . "\n", $prev_values);
+ # its type 'B' -> binary * -> fuss
+ if($types =~ m/(^_*)(B)$/) {
+ CHK("ARITHMETIC_CONTINUATIONS",
+ "Arithmetic expressions should be on one line\n" . $hereprev);
+ }
+
+ }
+# check for /,% and * at beginning of a line but exclude *var
+ if ($rawline =~ /^\+\s*(\*|\/|%).*$/) {
+ # need to check the type here
+ my $opline = $line; $opline =~ s/^./ /;
+ my ($curr_values, $types) = annotate_values($opline . "\n", $prev_values);
+ # first entry is type 'B' -> binary * -> fuss
+ if($types =~ m/(^_*)(B)/) {
+ 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
On Sun, 2015-05-10 at 13:26 +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.
The "if they can fit" is an important consideration
not addressed by this patch.
> This extension should flag such lines as CHECK only, as this is not
> mandated by the CodingStyle and in some cases they seem justified
> (e.g. kernel/sched/fair.c:760 and a few other examples in that file)
> TODO: Move to "# Check operator spacing" section as Joe Perches
> requested - simply got completely lost in that section...
> Also not clear if it really is the right place to put it -
> spacing issues and split operators are two quite different
> problems.
> Question: There are many (518) "lines over 80 char" warnings but that
> seems to be accepted in checkpatch.pl - should those be
> wrapped ?
perl is not c
Regexs are often quite long so I think trying to
do 80 column wrapping is something that would make
checkpatch even less readable.
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -2577,6 +2577,58 @@ sub process {
> "Logical continuations should be on the previous line\n" . $hereprev);
> }
>
> +# check for + or - at the end of a line but ignore --/++
> + if (!($rawline =~ /^\+.*(\+\+|--).*$/) &&
> + $rawline =~ /^\+.*(\+|-)$/) {
> + my $opline = $line; $opline =~ s/^./ /;
> + my ($curr_values, $types) = annotate_values($opline . "\n", $prev_values);
> + # its type and the end 'B' -> binary * -> fuss
> + if($types =~ m/^.*B$/) {
> + CHK("ARITHMETIC_CONTINUATIONS",
> + "Arithmetic expressions should be on one line\n" . $hereprev);
This message is unclear.
Arithmetic should not be on one line as that conflicts
with line length limits.
This test is similar to the logical operator continuation
test at line 2574.
Maybe the message would be clearer with
"Arithmetic operator should be on the previous line\n"
But more globally, maybe this style and message should
be used for many different operators:
o multiplicative (*, /, %)
o additive (+, -)
o bitwise shift (<<, >>)
o relational (<, >, <=, >=)
o equality (==, !=)
o bitwise (&, ^, |)
o logical (&&, ||)
o assignment (=, *=, /=, %=, +=, -=, <<=, >>=, &=, ^=, |=)
That's a rather bigger change though and would
be better done with more discussion first.
On Sun, 10 May 2015, Joe Perches wrote:
> On Sun, 2015-05-10 at 13:26 +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.
>
> The "if they can fit" is an important consideration
> not addressed by this patch.
>
Still playing with that but differenciating thos that would trivailly
fit by folding lines (line1 offset + line 1 content + 2 content < 80) and
those that should be partially rearanged like:
simple folding case:
func(var1 +
var2)
->
func(var1 + var2) < 80 char
rearange case:
func(..., var1 +
var2, ...)
->
func(...,
var1 + var2,
...)
are hard to pick appart (atleast I did not come up with a resonable solution)
- so that is not yet working (forgot that in the posted TODO list)
> > This extension should flag such lines as CHECK only, as this is not
> > mandated by the CodingStyle and in some cases they seem justified
> > (e.g. kernel/sched/fair.c:760 and a few other examples in that file)
>
> > TODO: Move to "# Check operator spacing" section as Joe Perches
> > requested - simply got completely lost in that section...
> > Also not clear if it really is the right place to put it -
> > spacing issues and split operators are two quite different
> > problems.
> > Question: There are many (518) "lines over 80 char" warnings but that
> > seems to be accepted in checkpatch.pl - should those be
> > wrapped ?
>
> perl is not c
>
> Regexs are often quite long so I think trying to
> do 80 column wrapping is something that would make
> checkpatch even less readable.
>
ok - just wanted to make sure since checkpatch reported 8 warnings
for the patch I sent out - thanks for the clarification.... and
anything that prevents checkpatch.ol from getting less readable is
absolutely justified...
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> []
> > @@ -2577,6 +2577,58 @@ sub process {
> > "Logical continuations should be on the previous line\n" . $hereprev);
> > }
> >
> > +# check for + or - at the end of a line but ignore --/++
> > + if (!($rawline =~ /^\+.*(\+\+|--).*$/) &&
> > + $rawline =~ /^\+.*(\+|-)$/) {
> > + my $opline = $line; $opline =~ s/^./ /;
> > + my ($curr_values, $types) = annotate_values($opline . "\n", $prev_values);
> > + # its type and the end 'B' -> binary * -> fuss
> > + if($types =~ m/^.*B$/) {
> > + CHK("ARITHMETIC_CONTINUATIONS",
> > + "Arithmetic expressions should be on one line\n" . $hereprev);
>
> This message is unclear.
>
> Arithmetic should not be on one line as that conflicts
> with line length limits.
>
> This test is similar to the logical operator continuation
> test at line 2574.
>
> Maybe the message would be clearer with
> "Arithmetic operator should be on the previous line\n"
>
for the case of (operand op)* being too long that would fit but I guess the
goal would primarily be to flag arithmetic expression that would actually
fit if rearanged - in wich case it makes no difference to which line you
move it.
fund(param1,
var1 - var2 * var3,
...
would be fine probably - and in this sense the arithmetic expression is
prefereablly on one line.
"Arithmetic operator are preferably placed on one line\n"
any better (for the expression < 80 char case) ?
> But more globally, maybe this style and message should
> be used for many different operators:
>
> o multiplicative (*, /, %)
> o additive (+, -)
> o bitwise shift (<<, >>)
> o relational (<, >, <=, >=)
> o equality (==, !=)
> o bitwise (&, ^, |)
> o logical (&&, ||)
> o assignment (=, *=, /=, %=, +=, -=, <<=, >>=, &=, ^=, |=)
>
> That's a rather bigger change though and would
> be better done with more discussion first.
>
yup - and in that case it might be better if its not a newbe that takes
on the task - even if it is enjoyable to some extent. An though the patch
looks simple it was quite painful and some parts are actually not clean
(like the way I filtered out the op type).
So for any such generalization it probably needs a generic operator type
function (some refined version of annotate_values()) and some additional
positional checking to not report cases that won't fit no mater what line
they are pushed to - so some classifier of the length of (operand operator)
sequence as a whole allowing to adjust the message then for the different
cases.
thx!
hofrat