Re: [PATCH v3 1/9] tests: introduce tree-wide code style checking

[Peter Maydell]

On Wed, 20 Jul 2022 at 17:32, Daniel P. Berrangé <berrange@redhat.com> wrote:
> I would certainly like to see us eventually remove
> checkpatch.pl because of the various downsides it has.
>
> The caveat is that I've not actually looked in any detail
> at what things checkpatch.pl actually checks for. Without
> looking my guess-timate is that we could probably replace
> 90% of it without much trouble. Perhaps we'll just decide
> some of the checkjs in checkpatch aren't worth the burden
> of maintaining its usage.

I went through checkpatch, and here are the warnings I think
are worth making sure we still have. I've included all the
ones that look like we've added them specifically for QEMU
on the basis that if we cared enough to edit checkpatch to
add them then we ought to care enough to retain them.

* "Do not add expected files together with tests,
   follow instructions in tests/qtest/bios-tables-test.c"
* "do not set execute permissions for source files"
* "please use python3 interpreter"
* "Author email address is mangled by the mailing list"
* syntax checks on Signed-off-by lines
* "does MAINTAINERS need updating?"
* "Invalid UTF-8"
* "trailing whitespace"
* "DOS line endings" (maybe)
* "Don't use '#' flag of printf format ('%#') in trace-events"
* "Hex numbers must be prefixed with '0x' [in trace-events]"
* line-length checks (though for a "whole codebase must pass"
  test we would want to set the length longer than the current
  "author should consider whether to wrap" value
* hard coded tabs
* the various dodgy-indentation checks
* the various space-required checks eg around operators
* misformatted block comments
* "do not use C99 // comments"
* "do not initialise globals/statics to 0 or NULL"
* "do not use assignment in if condition"
* "braces {} are necessary for all arms of this statement"
* "braces {} are necessary even for single statement blocks"
* "Use of volatile is usually wrong, please add a comment"
* "g_free(NULL) is safe this check is probably not required"
* "memory barrier without comment"
* "unnecessary cast may hide bugs, use g_new/g_renew instead"
* "consider using g_path_get_$1() in preference to g_strdup($1())"
* "use g_memdup2() instead of unsafe g_memdup()"
* "consider using qemu_$1 in preference to $1" (strto* etc)
* "use sigaction to establish signal handlers; signal is not portable"
* "please use block_init(), type_init() etc. instead of module_init()"
* "initializer for struct $1 should normally be const"
* "Error messages should not contain newlines"
* "use ctz32() instead of ffs()"
* ditto, ffsl, ffsll, bzero, getpagesize, _SC_PAGESIZE
* "Use g_assert or g_assert_not_reached" [instead of non-exiting glib asserts]

These seem to me to fall into three categories:

(1) many are easy enough to do with grep
(2) there are some checks we really do want to do on the patch,
not on the codebase (most obviously things like Signed-off-by:
format checks)
(3) there are coding style checks that do need to have at least some
idea of C syntax, to check brace placement, whitespace, etc

thanks
-- PMM