* [Qemu-devel] [PATCH RFC] scripts/checkpatch.pl: Bug fix
@ 2018-03-15 11:45 Su Hang
2018-03-15 11:50 ` Peter Maydell
0 siblings, 1 reply; 6+ messages in thread
From: Su Hang @ 2018-03-15 11:45 UTC (permalink / raw)
To: eblake, vsementsov; +Cc: qemu-devel
Bug fix: checkpatch.pl stops complaining about following pattern:
"""
do {
//do somethins;
} while (conditions);
"""
Two things need to be mentioned:
1) Before I casue this bug, checkpatch.pl will raise a wrong
complain:
"""
ERROR: braces {} are necessary even for single statement blocks
+ for (i == 0; i < 0; ++i)
+ {
+ ;
+ } else
"""
In this patch, this wrong complain get fixed.
2) Becasue all(`if`, `while`, `for`) check have been done in this
`if` block(Line: 2356), and this block contains following statement:
""" Line: 2379
$suppress_ifbraces{$ln + $offset} = 1;
"""
So the block after this block may never run:
""" Line: 2415
if (!defined $suppress_ifbraces{$linenr - 1} &&
$line =~ /\b(if|while|for|else)\b/ &&
$line !~ /\#\s*if/ &&
$line !~ /\#\s*else/) {
"""
But I think, maybe it's ok, becasue all check has been done by
up block. I'm not sure, please give me some advice.
Signed-off-by: Su Hang <suhang16@mails.ucas.ac.cn>
---
scripts/checkpatch.pl | 15 +++++++++++++--
1 file changed, 13 insertions(+), 2 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index d1fe79bcc47c..2ca833f22e5a 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2355,6 +2355,18 @@ sub process {
# check for missing bracing around if etc
if ($line =~ /(^.*)\b(?:if|while|for)\b/ &&
$line !~ /\#\s*if/) {
+ my $allowed = 0;
+
+ # Check the pre-context.
+ if ($line =~ /(\}.*?)$/) {
+ my $pre = $1;
+
+ if ($line !~ /else/) {
+ print "APW: ALLOWED: pre<$pre> line<$line>\n"
+ if $dbg_adv_apw;
+ $allowed = 1;
+ }
+ }
my ($level, $endln, @chunks) =
ctx_statement_full($linenr, $realcnt, 1);
if ($dbg_adv_apw) {
@@ -2363,7 +2375,6 @@ sub process {
if $#chunks >= 1;
}
if ($#chunks >= 0 && $level == 0) {
- my $allowed = 0;
my $seen = 0;
my $herectx = $here . "\n";
my $ln = $linenr - 1;
@@ -2407,7 +2418,7 @@ sub process {
$allowed = 1;
}
}
- if ($seen != ($#chunks + 1)) {
+ if ($seen != ($#chunks + 1) && !$allowed) {
ERROR("braces {} are necessary for all arms of this statement\n" . $herectx);
}
}
--
2.7.4
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH RFC] scripts/checkpatch.pl: Bug fix
2018-03-15 11:45 [Qemu-devel] [PATCH RFC] scripts/checkpatch.pl: Bug fix Su Hang
@ 2018-03-15 11:50 ` Peter Maydell
2018-03-15 12:06 ` Su Hang
0 siblings, 1 reply; 6+ messages in thread
From: Peter Maydell @ 2018-03-15 11:50 UTC (permalink / raw)
To: Su Hang; +Cc: Eric Blake, Vladimir Sementsov-Ogievskiy, QEMU Developers
On 15 March 2018 at 11:45, Su Hang <suhang16@mails.ucas.ac.cn> wrote:
> Bug fix: checkpatch.pl stops complaining about following pattern:
> """
> do {
> //do somethins;
> } while (conditions);
> """
>
> Two things need to be mentioned:
> 1) Before I casue this bug, checkpatch.pl will raise a wrong
> complain:
> """
> ERROR: braces {} are necessary even for single statement blocks
> + for (i == 0; i < 0; ++i)
> + {
> + ;
> + } else
> """
This is a bit of an odd example -- for() loops don't
take "else" clauses, so what is the 'else' doing here?
(Also, for QEMU style the "{" should be on the same line as the for(),
not on a line of its own.)
thanks
-- PMM
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH RFC] scripts/checkpatch.pl: Bug fix
2018-03-15 11:50 ` Peter Maydell
@ 2018-03-15 12:06 ` Su Hang
0 siblings, 0 replies; 6+ messages in thread
From: Su Hang @ 2018-03-15 12:06 UTC (permalink / raw)
To: peter maydell; +Cc: eric blake, vladimir sementsov-ogievskiy, qemu developers
Sorry about inappropriate example, what I want to express is,
when checkpatch.pl find pattern like this:
"""
for (i = 0; i < 0; ++i)
{
;
}
"""
It should raise
"""
ERROR: that open brace { should be on the previous line
#4: FILE: test.c:4:
+ for (i = 0; i < 0; ++i)
+ {
"""
Instead of:
"""
ERROR: braces {} are necessary even for single statement blocks
#4: FILE: test.c:4:
+ for (i = 0; i < 0; ++i)
+ {
+ ;
+ }
"""
> -----Original Messages-----
> From: "Peter Maydell" <peter.maydell@linaro.org>
> Sent Time: 2018-03-15 19:50:55 (Thursday)
> To: "Su Hang" <suhang16@mails.ucas.ac.cn>
> Cc: "Eric Blake" <eblake@redhat.com>, "Vladimir Sementsov-Ogievskiy" <vsementsov@virtuozzo.com>, "QEMU Developers" <qemu-devel@nongnu.org>
> Subject: Re: [Qemu-devel] [PATCH RFC] scripts/checkpatch.pl: Bug fix
>
> On 15 March 2018 at 11:45, Su Hang <suhang16@mails.ucas.ac.cn> wrote:
> > Bug fix: checkpatch.pl stops complaining about following pattern:
> > """
> > do {
> > //do somethins;
> > } while (conditions);
> > """
> >
> > Two things need to be mentioned:
> > 1) Before I casue this bug, checkpatch.pl will raise a wrong
> > complain:
> > """
> > ERROR: braces {} are necessary even for single statement blocks
> > + for (i == 0; i < 0; ++i)
> > + {
> > + ;
> > + } else
> > """
>
> This is a bit of an odd example -- for() loops don't
> take "else" clauses, so what is the 'else' doing here?
>
> (Also, for QEMU style the "{" should be on the same line as the for(),
> not on a line of its own.)
>
> thanks
> -- PMM
^ permalink raw reply [flat|nested] 6+ messages in thread
* [Qemu-devel] [PATCH RFC] scripts/checkpatch.pl: Bug fix
@ 2018-03-19 13:56 Su Hang
2018-03-19 15:25 ` Eric Blake
0 siblings, 1 reply; 6+ messages in thread
From: Su Hang @ 2018-03-19 13:56 UTC (permalink / raw)
To: eblake, vsementsov; +Cc: qemu-devel
Bug fix: checkpatch.pl stops complaining about following pattern:
"""
do {
//do somethins;
} while (conditions);
"""
Signed-off-by: Su Hang <suhang16@mails.ucas.ac.cn>
---
scripts/checkpatch.pl | 15 +++++++++++++--
1 file changed, 13 insertions(+), 2 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index d1fe79bcc47c..2ca833f22e5a 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2355,6 +2355,18 @@ sub process {
# check for missing bracing around if etc
if ($line =~ /(^.*)\b(?:if|while|for)\b/ &&
$line !~ /\#\s*if/) {
+ my $allowed = 0;
+
+ # Check the pre-context.
+ if ($line =~ /(\}.*?)$/) {
+ my $pre = $1;
+
+ if ($line !~ /else/) {
+ print "APW: ALLOWED: pre<$pre> line<$line>\n"
+ if $dbg_adv_apw;
+ $allowed = 1;
+ }
+ }
my ($level, $endln, @chunks) =
ctx_statement_full($linenr, $realcnt, 1);
if ($dbg_adv_apw) {
@@ -2363,7 +2375,6 @@ sub process {
if $#chunks >= 1;
}
if ($#chunks >= 0 && $level == 0) {
- my $allowed = 0;
my $seen = 0;
my $herectx = $here . "\n";
my $ln = $linenr - 1;
@@ -2407,7 +2418,7 @@ sub process {
$allowed = 1;
}
}
- if ($seen != ($#chunks + 1)) {
+ if ($seen != ($#chunks + 1) && !$allowed) {
ERROR("braces {} are necessary for all arms of this statement\n" . $herectx);
}
}
--
2.7.4
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH RFC] scripts/checkpatch.pl: Bug fix
2018-03-19 13:56 Su Hang
@ 2018-03-19 15:25 ` Eric Blake
2018-03-19 15:50 ` Su Hang
0 siblings, 1 reply; 6+ messages in thread
From: Eric Blake @ 2018-03-19 15:25 UTC (permalink / raw)
To: Su Hang, vsementsov; +Cc: qemu-devel
On 03/19/2018 08:56 AM, Su Hang wrote:
> Bug fix: checkpatch.pl stops complaining about following pattern:
> """
> do {
> //do somethins;
s/somethins/something/
> } while (conditions);
Having the commit message point to the commit id that introduced the bug
is useful. The grammar is awkward (it sounds like the bug is that
checkpatch.pl stopped complaining about the pattern, so the fix is to
reinstate the complaint); better might be:
Commit XYZ introduced a regression: checkpatch.pl started complaining
about the following valid pattern:
do {
/* something */
} while (condition);
Fix the script to once again permit this pattern.
> """
>
> Signed-off-by: Su Hang <suhang16@mails.ucas.ac.cn>
> ---
Is this an updated revision to a patch posted earlier? If so, including
'v2' in the subject line (easy if you use 'git format-patch -v2' or 'git
send-email -v2'), and then using this space after the --- separator to
describe what changed since v1, makes life easier for reviewers.
> scripts/checkpatch.pl | 15 +++++++++++++--
> 1 file changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index d1fe79bcc47c..2ca833f22e5a 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -2355,6 +2355,18 @@ sub process {
> # check for missing bracing around if etc
> if ($line =~ /(^.*)\b(?:if|while|for)\b/ &&
> $line !~ /\#\s*if/) {
> + my $allowed = 0;
> +
> + # Check the pre-context.
> + if ($line =~ /(\}.*?)$/) {
> + my $pre = $1;
> +
> + if ($line !~ /else/) {
> + print "APW: ALLOWED: pre<$pre> line<$line>\n"
> + if $dbg_adv_apw;
> + $allowed = 1;
> + }
> + }
> my ($level, $endln, @chunks) =
> ctx_statement_full($linenr, $realcnt, 1);
> if ($dbg_adv_apw) {
> @@ -2363,7 +2375,6 @@ sub process {
> if $#chunks >= 1;
> }
> if ($#chunks >= 0 && $level == 0) {
> - my $allowed = 0;
> my $seen = 0;
> my $herectx = $here . "\n";
> my $ln = $linenr - 1;
> @@ -2407,7 +2418,7 @@ sub process {
> $allowed = 1;
> }
> }
> - if ($seen != ($#chunks + 1)) {
> + if ($seen != ($#chunks + 1) && !$allowed) {
> ERROR("braces {} are necessary for all arms of this statement\n" . $herectx);
> }
> }
> --
> 2.7.4
>
>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH RFC] scripts/checkpatch.pl: Bug fix
2018-03-19 15:25 ` Eric Blake
@ 2018-03-19 15:50 ` Su Hang
0 siblings, 0 replies; 6+ messages in thread
From: Su Hang @ 2018-03-19 15:50 UTC (permalink / raw)
To: eric blake; +Cc: vsementsov, qemu-devel
> -----Original Messages-----
> From: "Eric Blake" <eblake@redhat.com>
> Sent Time: 2018-03-19 23:25:20 (Monday)
> To: "Su Hang" <suhang16@mails.ucas.ac.cn>, vsementsov@virtuozzo.com
> Cc: qemu-devel@nongnu.org
> Subject: Re: [Qemu-devel] [PATCH RFC] scripts/checkpatch.pl: Bug fix
> Having the commit message point to the commit id that introduced the bug
> is useful. The grammar is awkward (it sounds like the bug is that
> checkpatch.pl stopped complaining about the pattern, so the fix is to
> reinstate the complaint); better might be:
>
> Commit XYZ introduced a regression: checkpatch.pl started complaining
> about the following valid pattern:
> do {
> /* something */
> } while (condition);
>
> Fix the script to once again permit this pattern.
Thank you for correcting my mistakes. :-)
> Is this an updated revision to a patch posted earlier? If so, including
> 'v2' in the subject line (easy if you use 'git format-patch -v2' or 'git
> send-email -v2'), and then using this space after the --- separator to
> describe what changed since v1, makes life easier for reviewers.
Yes, it's an updated revision to a earlier patch.
But I wasn't about sending it, when I was using [ctrl + r] and [enter],
I send it accidentally.
Thanks for your kind suggestion.
Best,
Su Hang
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-03-19 15:50 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-03-15 11:45 [Qemu-devel] [PATCH RFC] scripts/checkpatch.pl: Bug fix Su Hang
2018-03-15 11:50 ` Peter Maydell
2018-03-15 12:06 ` Su Hang
-- strict thread matches above, loose matches on Subject: below --
2018-03-19 13:56 Su Hang
2018-03-19 15:25 ` Eric Blake
2018-03-19 15:50 ` Su Hang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).