Re: [PATCH v2 1/7] tests: introduce tree-wide code style checking

[Daniel P . Berrangé]

On Mon, Jul 04, 2022 at 04:46:53PM +0100, Peter Maydell wrote:
> On Mon, 4 Jul 2022 at 16:23, Daniel P. Berrangé <berrange@redhat.com> wrote:
> >
> > Historically QEMU has used the 'scripts/checkpatch.pl' script to
> > validate various style rules but there are a number of issues:
> 
> >  meson.build              |   3 +
> >  tests/Makefile.include   |   3 +-
> >  tests/meson.build        |  19 +++
> >  tests/style-excludes.mak |   4 +
> >  tests/style-infra.mak    | 265 +++++++++++++++++++++++++++++++++++++++
> >  tests/style.mak          |  24 ++++
> 
> From my point of view the main issue with checkpatch.pl is
> that nobody in the QEMU developers particularly understands
> it or is enthusiastic about trying to add more tests to it
> or adjust the existing ones where QEMU style diverges from
> the kernel style (but nor are we tracking and upgrading to
> newer versions of the kernel's script).
> 
> This seems to be aiming to replace a complex and hard to
> understand perl script with a complex and hard to understand
> makefile. I can't say I'm terribly enthusiastic :-/

I think the downsides comapred here are rather different orders of
magnitude. The checkpatch.pl script is 3000 lines of code where we
have years of experiance that no one in QEMU likes touching it.

The makefile here is 265 lines of which 50% is comments of license
text.  In terms of what contributors most care about though, is
how you add new rules, and most of the time that's involves just
adding a 3 line make rule based off a regex to match the code
pattern you want to prohibit. Some of this is a bit crufty to
look at, but we've got years of experiance in libvirt with many
contributors frequently adding new tests.

It only gets hairy if the pattern you're trying to forbid needs
to match across multiple lines of text - hence the difference in
complexity between matching 'osdep.h' exists in .c, vs 'osdep.h'
exists as the very first #include.  IME, the single-line matches
are most typical need that is addressed.

So while I wont claim this proposal is perfect, IMHO this would
be a significant step fowards over checkpatch.pl.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|