[Qemu-devel] [PATCH] checkpatch: downgrade "architecture specific defines should be avoided"

[Qemu-devel] [PATCH] checkpatch: downgrade "architecture specific defines should be avoided"

[Paolo Bonzini]

---
 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 =~ m@^.\s*\#\s*if.*\b__@) {
-			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
-- 
2.7.4

Re: [Qemu-devel] [PATCH] checkpatch: downgrade "architecture specific defines should be avoided"

[no-reply]

Hi,

Your series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 1474476607-4990-1-git-send-email-pbonzini@redhat.com
Subject: [Qemu-devel] [PATCH] checkpatch: downgrade "architecture specific defines should be avoided"

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

# Useful git options
git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git show --no-patch --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 - [tag update]      patchew/1474435433-9029-1-git-send-email-david@gibson.dropbear.id.au -> patchew/1474435433-9029-1-git-send-email-david@gibson.dropbear.id.au
 * [new tag]         patchew/1474476607-4990-1-git-send-email-pbonzini@redhat.com -> patchew/1474476607-4990-1-git-send-email-pbonzini@redhat.com
 * [new tag]         patchew/147447700612.30952.9420141963781948805.stgit@bahia -> patchew/147447700612.30952.9420141963781948805.stgit@bahia
 * [new tag]         patchew/1474477573-6386-1-git-send-email-lvivier@redhat.com -> patchew/1474477573-6386-1-git-send-email-lvivier@redhat.com
Switched to a new branch 'test'
ada0573 checkpatch: downgrade "architecture specific defines should be avoided"

=== OUTPUT BEGIN ===
Checking PATCH 1/1: checkpatch: downgrade "architecture specific defines should be avoided"...
ERROR: line over 90 characters
#18: FILE: scripts/checkpatch.pl:2410:
+			WARN("architecture specific defines should be avoided\n" .  $herecurr);

ERROR: Missing Signed-off-by: line(s)

total: 2 errors, 0 warnings, 8 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org

Re: [Qemu-devel] [PATCH] checkpatch: downgrade "architecture specific defines should be avoided"

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> 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 =~ m@^.\s*\#\s*if.*\b__@) {
> -			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.

Re: [Qemu-devel] [PATCH] checkpatch: downgrade "architecture specific defines should be avoided"

[Paolo Bonzini]

On 22/09/2016 09:12, Markus Armbruster wrote:
> Paolo Bonzini <pbonzini@redhat.com> 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 =~ m@^.\s*\#\s*if.*\b__@) {
>> -			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

Re: [Qemu-devel] [PATCH] checkpatch: downgrade "architecture specific defines should be avoided"

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 22/09/2016 09:12, Markus Armbruster wrote:
>> Paolo Bonzini <pbonzini@redhat.com> 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 =~ m@^.\s*\#\s*if.*\b__@) {
>>> -			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.