Attention is currently required from: Nico Huber, Martin Roth, Felix Held. Julius Werner 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 […]
Yeah, unfortunately I haven't got any attention at all on LKML yet. Maybe it would help if you post these there, create a bit more back-and-forth on the patch series to bump it to the top of the maintainer's inbox.
File util/lint/checkpatch.pl:
https://review.coreboot.org/c/coreboot/+/51838/comment/67fa9966_cb625ad9 PS2, Line 1345: @stack = (['', 0]) if ($#stack == -1);
Looks like this wouldn't be needed anymore? With the new index checks, […]
Right, I guess I could drop it.
https://review.coreboot.org/c/coreboot/+/51838/comment/d16b0ec5_c19f7ed6 PS2, Line 1553: $level = $stack[$#stack - 1];
Looks like this suffers the same?
Yeah, looks like it. I haven't actually looked into what this set of functions does and where it is used. But in isolation this looks like the same bug.
https://review.coreboot.org/c/coreboot/+/51838/comment/9a595e55_cf2364b1 PS2, Line 3629: s
The `s` modifier looks suspicious. AIUI, it means the `.` could match […]
Probably, yeah. This is from Ivo's original patch which I didn't want to modify upstream for attribution reasons. Maybe he thought that without the /s he couldn't match "\n" at all (some regex engines work that way, although I think Perl's doesn't... at least the thing for labels I wrote below seemed to work without it).
If you think it's important enough I can put another cleanup patch behind Ivo's to fix up minor stuff.
https://review.coreboot.org/c/coreboot/+/51838/comment/a397bca7_a39e6180 PS2, Line 3638: \s*
Is there a point to match preceding whitespace if we don't match from the start?
No idea, this is also Ivo's stuff. Probably not.
https://review.coreboot.org/c/coreboot/+/51838/comment/0b0befbd_2a32ab6c PS2, Line 3642: \s*?
I couldn't find any example where this matches. […]
I think ctx_statement_block() just doesn't handle do-while perfectly, and this is meant to work around that. When parsing a do-while statement, $stat ends up counting the `do { ... }` up until the closing brace and then $s_next gets the ` while (...)`, starting with the leading space which makes it look indented. This will eat the while part so that s_next really captures the next statement. (That's why it's using `^` instead of `\n`, too, because it's really only about the start of the whole statement.)
https://review.coreboot.org/c/coreboot/+/51838/comment/3972f73b_a15404dc PS2, Line 3663: stat_real
Nit, somehow $stat_real is missing the closing brace (if it was a block with braces).
Hmm... yeah, not sure, $stat_real already falls out of the existing code above. This is printing it the same way the existing warning is printing it, I hope that's good enough.
-
Julius Werner (Code Review)