Re: [PATCH] [checkpatch.pl] ctx_statement_block #if/#else/#endif fix
On Wed, Jun 11, 2014 at 10:57:11AM +0100, Andy Whitcroft wrote: > Signed-off-by: Andy Whitcroft With your permissions I'll add a Reported-by: etc to this patch in your name. -apw -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] [checkpatch.pl] ctx_statement_block #if/#else/#endif fix
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 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 --- 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] [checkpatch.pl] ctx_statement_block #if/#else/#endif fix
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 > --- > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] [checkpatch.pl] ctx_statement_block #if/#else/#endif fix
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 : > > 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 > > --- > > > > 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 > > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] [checkpatch.pl] ctx_statement_block #if/#else/#endif fix
Andy, Joe, What do you think about my patchset below? Regards, Ivo Sieben 2014-05-15 16:43 GMT+02:00 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 > --- > > 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 > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] [checkpatch.pl] ctx_statement_block #if/#else/#endif fix
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 --- 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/