On Thu, May 15, 2014 at 04:43:15PM +0200, Ivo Sieben wrote:
> When picking up a complete statement block #if/#else/#endif prepocesor
> boundaries are taken into account by pushing current level & type on a stack.
> But on an #else the level was read from stack again (without actually popping it
> from stack) causing the statement block to end too early on the next ';'.
> Fixed this.
>
> For example the following code:
>
> if (!test()) {
> #ifdef NEVER
> foo();
> bar();
> #else
> bar();
> foo();
> #endif
> }
>
> Results in statement block:
>
> STATEMENT<+ if (!test()) {
> +#ifdef NEVER
> + foo();
> + bar();
> +#else
> + bar();>
> CONDITION<+ if (!test())>
>
> While you would expect:
>
> STATEMENT<+ if (!test()) {
> +#ifdef NEVER
> + foo();
> + bar();
> +#else
> + bar();
> + foo();
> +#endif
> + }>
> CONDITION<+ if (!test())>
>
> Signed-off-by: Ivo Sieben <[email protected]>
> ---
>
> Request for comments:
> I think I fixed a problem here that I encountered while I was working on another
> changeset in which I check the statement block after a condition.
> Somehow the statement block did not contain everything I expected.
>
> scripts/checkpatch.pl | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 34eb216..e7bca89 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -878,7 +878,7 @@ sub ctx_statement_block {
> if ($remainder =~ /^#\s*(?:ifndef|ifdef|if)\s/) {
> push(@stack, [ $type, $level ]);
> } elsif ($remainder =~ /^#\s*(?:else|elif)\b/) {
> - ($type, $level) = @{$stack[$#stack - 1]};
> + # no changes to stack: type & level remain the same
> } elsif ($remainder =~ /^#\s*endif\b/) {
> ($type, $level) = @{pop(@stack)};
> }
> @@ -1050,7 +1050,7 @@ sub ctx_block_get {
> if ($lines[$line] =~ /^.\s*#\s*(?:ifndef|ifdef|if)\s/) {
> push(@stack, $level);
> } elsif ($lines[$line] =~ /^.\s*#\s*(?:else|elif)\b/) {
> - $level = $stack[$#stack - 1];
> + # no changes to stack: type & level remain the same
> } elsif ($lines[$line] =~ /^.\s*#\s*endif\b/) {
> $level = pop(@stack);
> }
> --
> 1.7.9.5
It doesn't seem right to remove the restore of the state. If you
consider the effect of an #if/#else/#endif combination the state of the
statement parser should be reset to the state at #if at the #else as the
code there is an alternative for the code in the other branch(s) of the
preprocessor, either is a continuation of the statement in progress when
reached. This contrived example should make it clear we need to
restore:
c = (d +
#if A
a)
#else
b)
#endif
That said it appears to be doing something wrong in your example.
/me will have a poke and get back to you.
-apw
On Wed, Jun 11, 2014 at 10:16:40AM +0100, Andy Whitcroft wrote:
> It doesn't seem right to remove the restore of the state. If you
> consider the effect of an #if/#else/#endif combination the state of the
> statement parser should be reset to the state at #if at the #else as the
> code there is an alternative for the code in the other branch(s) of the
> preprocessor, either is a continuation of the statement in progress when
> reached. This contrived example should make it clear we need to
> restore:
>
> c = (d +
> #if A
> a)
> #else
> b)
> #endif
>
>
> That said it appears to be doing something wrong in your example.
>
> /me will have a poke and get back to you.
Ok, the fundamental issue seems to have been an off by one thinko in the
attempt to restore the state to the previous level ($@array is the index
of the late element not the length of the array). The patch below seems
to sort out the issue. Could you give it a try and confirm it covers
your primary cases, and if so I will push it to akpm.
Thanks.
-apw
>From b47b6f4934471bcc4d653b53be0950b9dc590c87 Mon Sep 17 00:00:00 2001
From: Andy Whitcroft <[email protected]>
Date: Wed, 11 Jun 2014 10:51:13 +0100
Subject: [PATCH] checkpatch: fix handling of stacked state for preprocessor if
branches
When reaching an alternative branch of a preprocessor directive we need to
reset statement state to that at the start of the if as this new segment
is logically ajacent to it. Currently we inadvertantly take the state
for the containing block (read off by one error). Pick the correct state.
This fixes statement parsing for the below style constructs:
if (!test()) {
#ifdef NEVER
foo();
bar();
#else
bar();
foo();
#endif
}
Signed-off-by: Andy Whitcroft <[email protected]>
---
scripts/checkpatch.pl | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 010b18e..d3c0126 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -885,7 +885,7 @@ sub ctx_statement_block {
if ($remainder =~ /^#\s*(?:ifndef|ifdef|if)\s/) {
push(@stack, [ $type, $level ]);
} elsif ($remainder =~ /^#\s*(?:else|elif)\b/) {
- ($type, $level) = @{$stack[$#stack - 1]};
+ ($type, $level) = @{$stack[$#stack]};
} elsif ($remainder =~ /^#\s*endif\b/) {
($type, $level) = @{pop(@stack)};
}
@@ -1057,7 +1057,7 @@ sub ctx_block_get {
if ($lines[$line] =~ /^.\s*#\s*(?:ifndef|ifdef|if)\s/) {
push(@stack, $level);
} elsif ($lines[$line] =~ /^.\s*#\s*(?:else|elif)\b/) {
- $level = $stack[$#stack - 1];
+ $level = $stack[$#stack];
} elsif ($lines[$line] =~ /^.\s*#\s*endif\b/) {
$level = pop(@stack);
}
--
2.0.0
On Wed, Jun 11, 2014 at 10:57:11AM +0100, Andy Whitcroft wrote:
> Signed-off-by: Andy Whitcroft <[email protected]>
With your permissions I'll add a Reported-by: etc to this patch in your
name.
-apw