qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH] checkpatch: downgrade "architecture specific de


From: Paolo Bonzini
Subject: Re: [Qemu-devel] [PATCH] checkpatch: downgrade "architecture specific defines should be avoided"
Date: Thu, 22 Sep 2016 11:15:57 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.3.0


On 22/09/2016 09:12, Markus Armbruster wrote:
> Paolo Bonzini <address@hidden> writes:
> 
>> ---
>>  scripts/checkpatch.pl | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
>> index dde3f5f..3afa19a 100755
>> --- a/scripts/checkpatch.pl
>> +++ b/scripts/checkpatch.pl
>> @@ -2407,7 +2407,7 @@ sub process {
>>  # we have e.g. CONFIG_LINUX and CONFIG_WIN32 for common cases
>>  # where they might be necessary.
>>              if ($line =~ address@hidden@) {
>> -                    ERROR("architecture specific defines should be 
>> avoided\n" .  $herecurr);
>> +                    WARN("architecture specific defines should be 
>> avoided\n" .  $herecurr);
>>              }
>>  
>>  # Check that the storage class is at the beginning of a declaration
> 
> git-grep finds almost 400 of them.  We certainly want people to think
> twice (or thrice) before they add more.  The question to discuss here is
> whether we want to force that thinking onto the list.  If yes, keep
> ERROR.  If no, downgrade to warn.

I actually count 450, but:

- about a 100 are in imported code (disas/libvixl,
include/standard-headers and linux-headers, disas)

- another 40-odd hits are __NR_* syscall numbers

- about 80 are in user-exec.c, block/raw-posix.c, util/oslib-posix.c,
util/qemu-openpty.c, util/qemu-thread-posix.c which is probably unavoidable

- another 30 are in tcg

So this already covers more than half the hits.


The patch is a bit of a heavy hammer, but I don't think it's an endemic
problem that warrants a complaint on the list.  If we want to keep the
error, I think we should have:

- a symbol blacklist.  For example __linux__ and _WIN32 can be trivially
replaced by CONFIG_LINUX and CONFIG_WIN32, and __GNUC__ is probably a
bad idea (but __clang__ not so much; clang defines __GNUC__ for an
absurdly old version).

- a file blacklist, for example I would not expect target-*/ and hw/
should not have __ symbols and in fact they hardly have any

and warn for everything else.  Something for bite-sized tasks?

Paolo



reply via email to

[Prev in Thread] Current Thread [Next in Thread]