Re: [Qemu-devel] [PATCH] checkpatch: downgrade "architecture specific defines should be avoided"
Paolo Bonzini writes: > On 22/09/2016 09:12, Markus Armbruster wrote: >> Paolo Bonzini 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.
Re: [Qemu-devel] [PATCH] checkpatch: downgrade "architecture specific defines should be avoided"
On 22/09/2016 09:12, Markus Armbruster wrote: > Paolo Bonzini 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"
Paolo Bonzini 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"
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-pbonz...@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-da...@gibson.dropbear.id.au -> patchew/1474435433-9029-1-git-send-email-da...@gibson.dropbear.id.au * [new tag] patchew/1474476607-4990-1-git-send-email-pbonz...@redhat.com -> patchew/1474476607-4990-1-git-send-email-pbonz...@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-lviv...@redhat.com -> patchew/1474477573-6386-1-git-send-email-lviv...@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-de...@freelists.org
[Qemu-devel] [PATCH] checkpatch: downgrade "architecture specific defines should be avoided"
--- 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