Message-ID: <3e7134b15bfbfbf47bf55fce733e8f986865f69b.camel@perches.com>
Date:   Sun, 28 Mar 2021 03:37:32 -0700
From:   Joe Perches <joe@...ches.com>
To:     Mihai Moldovan <ionic@...ic.de>,
        Randy Dunlap <rdunlap@...radead.org>,
        Masahiro Yamada <masahiroy@...nel.org>
Cc:     linux-kernel@...r.kernel.org
Subject: Re: [PATCH] kconfig: nconf: stop endless search-up loops

On Sun, 2021-03-28 at 11:27 +0200, Mihai Moldovan wrote:
> * On 3/27/21 11:26 PM, Randy Dunlap wrote:
> > There is a test for it in checkpatch.pl but I also used checkpatch.pl
> > without it complaining, so I don't know what it takes to make the script
> > complain.
> > 
> > 			if ($lead !~ /(?:$Operators|\.)\s*$/ &&
> > 			    $to !~ /^(?:Constant|[A-Z_][A-Z0-9_]*)$/ &&
> > 			    WARN("CONSTANT_COMPARISON",
> > 				 "Comparisons should place the constant on the right side of the test\n" . $herecurr) &&
> 
> There are multiple issues, err, "challenges" there:
>   - literal "Constant" instead of "$Constant"
>   - the left part is misinterpreted as an operation due to the minus sign
>     (arithmetic operator)
>   - $Constant is defined as "qr{$Float|$Binary|$Octal|$Hex|$Int}" (which is
>     okay), but all these types do not include their negative range.
> 
> As far as I can tell, the latter is intentional. Making these types compatible
> with negative values causes a lot of other places to break, so I'm not keen on
> changing this.
> 
> The minus sign being misinterpreted as an operator is highly difficult to fix
> in a sane manner. The original intention was to avoid misinterpreting
> expressions like (var - CONSTANT real_op ...) as a constant-on-left expression
> (and, more importantly, to not misfix it when --fix is given).
> 
> The general idea is sane and we probably shouldn't change that, but it would
> be good to handle negative values as well.
> 
> At first, I was thinking of overriding this detection by checking if the
> leading part matches "(-\s*$", which should only be true for negative values,
> assuming that there is always an opening parenthesis as part of a conditional
> statement/loop (if, while). After playing around with this and composing this
> message for a few hours, it dawned on me that there can easily be free-
> standing forms (for instance as part of for loops or assignment lines), so
> that wouldn't cut it.
> 
> It really goes downhill from here.
> 
> I assume that false negatives are nuisances due to stylistic errors in the
> code, but false positives actually harmful since a lot of them make code
> review by maintainers very tedious.
> 
> So far, minus signs were always part of the leading capture group. I'd
> actually like to have them in the constant capture group instead:
> 
> -		    $line =~ /^\+(.*)\b($Constant|[A-Z_][A-Z0-9_]*)\s*($Compare)\s*($LvalOrFunc)/) {
> +		    $line =~ /^\+(.*)(-?\s*$Constant|[A-Z_][A-Z0-9_]*)\s*($Compare)\s*($LvalOrFunc)/) {
> 
> With that sorted, the next best thing I could come up with to weed out
> preceding variables was this (which shouldn't influence non-negative
> constants):
> 
> -			if ($lead !~ /(?:$Operators|\.)\s*$/ &&
> +			if ($lead !~ /(?:$Operators|\.|[a-z])\s*$/ &&
> 
> 
> There still are a lot of expressions that won't match this, like
> "-1 + 0 == var" (i.e., "CONSTANT <arith_op> CONSTANT <op> ...") or
> constellations like a simple "(CONSTANT) <op> ..." (e.g.,
> "(1) == var").
> 
> This is all fuzzy and getting this right would involve moving away from
> trying to make sense of C code with regular expressions in Perl, but actually
> parsing it to extract the semantics. Not exactly something I'd like to do...
> 
> Thoughts on my workaround for this issue?

Making $Constant not include negative values was very intentional.

> Did I miss anything crucial or introduce a new bug inadvertently?

Not as far as I can tell from a trivial read.
My best guess as to what is best or necessary to do is nothing.
checkpatch isn't a real parser and never will be.

It can miss stuff.  It's imperfect.  It doesn't bother me.

Powered by blists - more mailing lists