Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/51551 )
Change subject: lint: checkpatch: Ignore ASSIGN_IN_IF and UNNECESSARY_ELSE errors ......................................................................
Patch Set 4:
(1 comment)
Patchset:
PS3:
This will stop Jenkins from creating Gerrit comments to warn about these two things, yes (at least I hope it does, that's my goal with this patch).
Sorry for not responding, Gerrit's mail got caught in a filter /o\
Removing UNNECESSARY_ELSE doesn't mean you *have* to always put the else, of course, it just means it's left up to the author and reviewers. Do you really think this warning is critical in protecting us from a deluge of over-indented code? Personally I don't recall seeing a lot of that before we introduced checkpatch, nor have I seen many patches recently where authors started out that way and then changed it due to the Jenkins warning. If you think it's a big concern, could you point to some examples?
I like to think of the checkpatch warnings as a nice hint for inexperienced people. I don't recall having witnessed anyone taking them too seriously. On my own changes, I mostly ignore them without trouble. Examples from the past before checkpatch would not reflect the constitution of our community today.
I'm just kinda tired of writing code that's perfectly fine according to our coding style and still get bothered about it by Jenkins, so I'm trying to reduce those cases. No matter how often we write "checkpatch is not authoritative" somewhere there's still always enough people who assume that it is or ask about it in reviews or otherwise create unnecessary churn (even if it's just when you don't know whether people are not giving you a +2 because they haven't reviewed the patch yet or they're implicitly waiting on you to "resolve" the spurious checkpatch comments). And the email spam also just gets annoying when you upload a lot of revisions for the same patch.
Jenkins' comments are automatically tagged resolved and I treat them like that. It's just a comment that something _might_ be better written another way. Maybe we should adapt the comment texts to reflect that?
If somebody would be waiting for style changes before starting a review, I would probably not want them to review my changes anyway.
(Also, note that checkpatch already has a separate DEEP_INDENTATION warning for 6+ tabs if that's the primary concern here.)
Should we make it 5+? ;) I don't think we need warnings for things that are obvious to everyone. It's rather the small nits that are seen more often than exceptionally bad style that I would prefer to have covered by a bot.