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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1431276724.29257.20.camel@perches.com>
Date:	Sun, 10 May 2015 09:52:04 -0700
From:	Joe Perches <joe@...ches.com>
To:	Nicholas Mc Guire <hofrat@...dl.org>
Cc:	Andy Whitcroft <apw@...onical.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH RFC V2] checkpatch: flag split arithmetic operations
 with CHECK

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.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/