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

[Nicholas Mc Guire <der.herr@hofr.at>]

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