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 3:
(5 comments)
Patchset:
PS3: Okay, sorry for not getting back to this in a while, I had a few very busy weeks. But I still think it's important that we finalize something here so we don't get in the situation one year from now again where someone is again upset about a bug creeping in and we discover that we had all these discussions before but didn't end up doing anything in the end.
Unfortunately, it seems that getting this into upstream checkpatch is pretty much impossible at the moment: one of the maintainers seems to have been fully AWOL for years, and the other one says that this code isn't his area and he'll not take it unless the AWOL maintainer reviews it first. I think the next best option is to just apply this locally then (I'm happy to sign up for maintaining this code through future uprevs if necessary). I've applied Nico's remaining comments now and retested it again, so if nobody else has any concerns this should be ready to go in.
As far as the GCC warning is concerned, looks like nothing really happened in that area either and I'm not sure if anyone is actively pursuing that? I tried it out quickly but I think the fact that it doesn't pick up on macros at all is a pretty big drawback -- even if we fixed printk(), there are plenty of other macros in coreboot. I'd rather having something with a very low chance of false positives (that you can then just ignore when they happen) than something which might miss a whole bunch of important cases. (As I mentioned before, on the current coreboot codebase there's not a single false positive.)
File util/lint/checkpatch.pl:
https://review.coreboot.org/c/coreboot/+/51838/comment/d1b23e25_bf22e5a0 PS2, Line 1345: @stack = (['', 0]) if ($#stack == -1);
Right, I guess I could drop it.
Dropped it, but I decided to initialize @stack like in ctx_statement_block instead so that the condition can remain $#stack > 0 and the code is more comparable between the two.
https://review.coreboot.org/c/coreboot/+/51838/comment/59adb260_5b46baf6 PS2, Line 3629: s
Probably, yeah. […]
Removed the `s`.
https://review.coreboot.org/c/coreboot/+/51838/comment/4ff96e55_08af42ba PS2, Line 3638: \s*
No idea, this is also Ivo's stuff. Probably not.
removed
https://review.coreboot.org/c/coreboot/+/51838/comment/d732d9f1_449310dd PS2, Line 3642: \s*?
I think ctx_statement_block() just doesn't handle do-while perfectly, and this is meant to work arou […]
Left as is for now. It seems to work in practice.
-
Julius Werner (Code Review)