[PATCH] [checkpatch.pl] ctx_statement_block #if/#else/#endif fix

[PATCH] [checkpatch.pl] ctx_statement_block #if/#else/#endif fix

[Ivo Sieben]

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 <meltedpianoman@gmail.com>
---

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

Re: [PATCH] [checkpatch.pl] ctx_statement_block #if/#else/#endif fix

[Ivo Sieben]

Andy, Joe,

What do you think about my patchset below?

Regards,
Ivo Sieben

2014-05-15 16:43 GMT+02:00 Ivo Sieben <meltedpianoman@gmail.com>:
> 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 <meltedpianoman@gmail.com>
> ---
>
> 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
>

Re: [PATCH] [checkpatch.pl] ctx_statement_block #if/#else/#endif fix

[Joe Perches]

On Tue, 2014-06-10 at 08:19 +0200, Ivo Sieben wrote:
> Andy, Joe,
> 
> What do you think about my patchset below?

Andy wrote that originally and I think he should respond.

> Regards,
> Ivo Sieben
> 
> 2014-05-15 16:43 GMT+02:00 Ivo Sieben <meltedpianoman@gmail.com>:
> > 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 <meltedpianoman@gmail.com>
> > ---
> >
> > 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
> >

Re: [PATCH] [checkpatch.pl] ctx_statement_block #if/#else/#endif fix

[Andy Whitcroft]

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 <meltedpianoman@gmail.com>
> ---
> 
> 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

Re: [PATCH] [checkpatch.pl] ctx_statement_block #if/#else/#endif fix

[Andy Whitcroft]

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 <apw@canonical.com>
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 <apw@canonical.com>
---
 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

Re: [PATCH] [checkpatch.pl] ctx_statement_block #if/#else/#endif fix

[Andy Whitcroft]

On Wed, Jun 11, 2014 at 10:57:11AM +0100, Andy Whitcroft wrote:

> Signed-off-by: Andy Whitcroft <apw@canonical.com>

With your permissions I'll add a Reported-by: etc to this patch in your
name.

-apw