2014-06-10 06:19:49

by Ivo Sieben

[permalink] [raw]
Subject: 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 <[email protected]>:
> 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
>


2014-06-10 16:24:13

by Joe Perches

[permalink] [raw]
Subject: 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 <[email protected]>:
> > 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
> >