Attention is currently required from: Martin Roth, Julius Werner, Felix Held. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/51838 )
Change subject: lint: checkpatch: Add SUSPICIOUS_CODE_INDENT test ......................................................................
Patch Set 2:
(7 comments)
Patchset:
PS2: Hi, hmmm, thought I'd give it a quick look and then... all the comments. Let me know if I should move that to the LKML.
File util/lint/checkpatch.pl:
https://review.coreboot.org/c/coreboot/+/51838/comment/a37a0834_b8ed86f8 PS2, Line 1345: @stack = (['', 0]) if ($#stack == -1); Looks like this wouldn't be needed anymore? With the new index checks, it would never be accessed, right?
Removing it would change the condition to `$#stack >= 0`, I guess.
https://review.coreboot.org/c/coreboot/+/51838/comment/fcb4bd3e_d3a5f0d0 PS2, Line 1553: $level = $stack[$#stack - 1]; Looks like this suffers the same?
https://review.coreboot.org/c/coreboot/+/51838/comment/55345311_1ab1004a PS2, Line 3629: s The `s` modifier looks suspicious. AIUI, it means the `.` could match a `\n` too. So in a string like "\n\n+\n" we'd remove the second `\n` and miss to remove the `+`.
I guess that wont't happen because it would be invalid input, but I also don't see what we need the `s` for.
https://review.coreboot.org/c/coreboot/+/51838/comment/98c41224_22a3a795 PS2, Line 3638: \s* Is there a point to match preceding whitespace if we don't match from the start?
https://review.coreboot.org/c/coreboot/+/51838/comment/026d10b4_399c788b PS2, Line 3642: \s*? I couldn't find any example where this matches.
First, it seems matching the start should be done with `\n`, not `^`, or we'd need the `m` modifier, I guess. I don't know why, the expressions above seem assume that too, though. But what's the point of the substitution? The only thing that is allowed to follow is a semicolon, AFAICS. If we continue, we'd check if a semicolon at the start of a line (because we just dropped all the indentation) is too much indented...
Also, why should we not warn if this is oddly indented?
If we don't want to warn, we should simply skip it `if ($stat =~ /\bdo\b/)`. Or is there anything else that could follow a `do {}`?
If we want to warn, I don't see why we should patch it.
https://review.coreboot.org/c/coreboot/+/51838/comment/16d6370b_29963c02 PS2, Line 3663: stat_real Nit, somehow $stat_real is missing the closing brace (if it was a block with braces).