Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
To:     Randy Dunlap <rdunlap@infradead.org>,
        Masahiro Yamada <masahiroy@kernel.org>
Cc:     linux-kernel@vger.kernel.org
References: <20210327120155.500-1-ionic@ionic.de>
 <1ca3a90f-799e-d917-370e-1475e33cdb14@infradead.org>
 <111c5853-e488-0aaa-18e9-36792b648427@ionic.de>
 <362edad0-daea-1f56-bed3-b1df174cbbf1@infradead.org>
From:   Mihai Moldovan <ionic@ionic.de>
Subject: Re: [PATCH] kconfig: nconf: stop endless search-up loops
Message-ID: <695102ca-0c05-7bf9-2758-08b5405e876b@ionic.de>
Date:   Sun, 28 Mar 2021 11:27:14 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.6.0
MIME-Version: 1.0
In-Reply-To: <362edad0-daea-1f56-bed3-b1df174cbbf1@infradead.org>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk

* 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? Did I miss anything crucial or
introduce a new bug inadvertently?