[coreboot] What is the best way to treat warnings reported by checkpatch.pl

[ron minnich]

A couple of questions preceded by the usual curmudgeonly comment :-)

I'm not a big fan of checkpatch.pl. It's 5965 lines of dense perl spaghetti
code, continues to grow, and is attempting to achieve that which I
understand may be impossible: parsing C with REs and random blobs of PERL.
Given that the problem may be impossible, checkpatch finds it hard, and
makes mistakes, as experienced on the projects that use it.

It's not surprising it continues to grow, and not  surprising it's got lots
of errors. You can find discussions on the various lists which boil down to
"just ignore checkpatch.pl in this case." This is not confidence-inspiring.

I'd most prefer to move to clang-format for this sort of thing, which is
what we're doing on at least one other kernel project I work on.

I continue to not understand the injunction on typedefs; operating systems
written by the inventors of C continued to use them until Bell Labs
disbanded the OS group in 2015 ...

Finally, why the use of packed? In general you really only want to use
packed when you're using it to directly address data. Why not deserialize
the data from memory into an unpacked struct? That's how ACPICA seems to do
it.

thanks

ron

On Tue, Jul 26, 2016 at 5:05 PM Julius Werner <jwerner at chromium.org> wrote:

> Does git commit --no-verify (or -n for short) allow you to commit?
>
> I think we should try to do a little of option 1 within reason, not by
> forking the script but rather by disabling more check steps that don't
> match the coreboot code style (by having our wrapper pass --ignore XXX
> flags to checkpatch) and possibly upstreaming checkpatch.pl patches
> with new features we need to the Linux kernel (to make the detection
> more accurate or add a more specific --ignore flag we can turn on). In
> the Chromium fork of coreboot we're already using a bunch of those
> flags that we should probably use in upstream coreboot as well:
>
> --ignore C99_COMMENTS
> --ignore GLOBAL_INITIALISERS
> --ignore INITIALISED_STATIC
> --ignore LINE_SPACING
> --ignore NEW_TYPEDEFS
> --ignore PREFER_ALIGNED
> --ignore PREFER_PACKED
> --ignore PREFER_PRINTF
> --ignore SPLIT_STRING
>
> However, in my experience there will always be edge cases checkpatch
> doesn't get right, no matter how well you tweak it. It should always
> be up to the discretion of the committer/reviewer which warnings they
> act upon, so we need to retain a mechanism (like --no-verify) to work
> around it if necessary. Maybe we should update the pre-commit hook to
> print an explanation to this effect. (Ideally, I think it would be
> awesome if Gerrit could separately run checkpatch and make it explicit
> in the interface if someone circumvented a warning this way so it can
> be discussed, but that's probably not and easy thing to add.)
>
> --
> coreboot mailing list: coreboot at coreboot.org
> https://www.coreboot.org/mailman/listinfo/coreboot
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.coreboot.org/pipermail/coreboot/attachments/20160727/9696a2d2/attachment.html>