[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] checkpatch: downgrade "architecture specific de
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH] checkpatch: downgrade "architecture specific defines should be avoided" |
Date: |
Thu, 22 Sep 2016 13:52:54 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
Paolo Bonzini <address@hidden> writes:
> 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?
Works for me.