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.
