[Cocci] coccinelle: trivial linux code style reformatting

[Cocci] coccinelle: trivial linux code style reformatting

[Joe Perches]

Hello.

I tried this little cocci script to remove unnecessary
uses of return in void functions.

I tried with 2 different versions of spatch and get
different output content, neither of which is the
preferred kernel style.

The output is apparently correct and compilable.
It is just not kernel style.  I'm not sure there
is a defect really, just wanted to bring it up.

Maybe the first transform could be better written.

$ cat void_return.cocci
@@
identifier func;
expression S;
@@
void func(...) {
...
	if (...)
-		return S;
+		{ S; return; }
...
}

@@
identifier func;
expression S;
@@
void func(...) {
...
	if (...) {
...
-		return S;
+		S;
	}
...
}

@@
identifier func;
expression S;
@@
void func(...) {
...
-	return S;
+	S;
}
$

$ ~/coccinelle/spatch --version
spatch version 1.0.0-rc20 without Python support and with PCRE support

$ ~/coccinelle/spatch -sp-file void_return.cocci drivers/gpu/drm/i915/i915_irq.c
init_defs_builtins: /usr/local/share/coccinelle/standard.h
HANDLING: drivers/gpu/drm/i915/i915_irq.c
diff = 
--- drivers/gpu/drm/i915/i915_irq.c
+++ /tmp/cocci-output-13176-0a96c4-i915_irq.c
@@ -2818,7 +2818,10 @@ static void i915_hangcheck_elapsed(unsig
 	}
 
 	if (rings_hung)
-		return i915_handle_error(dev, true, "Ring hung");
+	{
+		i915_handle_error(dev, true, "Ring hung");
+		return;
+	}
 
 	if (busy_count)
 		/* Reset timer case chip hangs without another request


and with an older spatch version

$ spatch --version
spatch version 1.0.0-rc14 without Python support and with PCRE support

$ spatch -sp-file void_return.cocci drivers/gpu/drm/i915/i915_irq.c
init_defs_builtins: /usr/local/share/coccinelle/standard.h
HANDLING: drivers/gpu/drm/i915/i915_irq.c
diff = 
--- drivers/gpu/drm/i915/i915_irq.c
+++ /tmp/cocci-output-13258-bc64a5-i915_irq.c
@@ -2817,8 +2817,11 @@ static void i915_hangcheck_elapsed(unsig
 		}
 	}
 
-	if (rings_hung)
-		return i915_handle_error(dev, true, "Ring hung");
+	if (rings_hung) {
+		
+			i915_handle_error(dev, true, "Ring hung");
+			return;		
+		}
 
 	if (busy_count)
 		/* Reset timer case chip hangs without another request

[Cocci] coccinelle: trivial linux code style reformatting

[Peter Senna Tschudin]

I changed the first rule to:

@@
identifier func;
expression S,E;
@@
void func(...) {
...
- if (E) return S;
+ if (E) {S; return;}
...
}

and it seem to work now, can you confirm it does the expected? I'm using rc20...

On Tue, Apr 1, 2014 at 7:42 PM, Joe Perches <joe@perches.com> wrote:
> Hello.
>
> I tried this little cocci script to remove unnecessary
> uses of return in void functions.
>
> I tried with 2 different versions of spatch and get
> different output content, neither of which is the
> preferred kernel style.
>
> The output is apparently correct and compilable.
> It is just not kernel style.  I'm not sure there
> is a defect really, just wanted to bring it up.
>
> Maybe the first transform could be better written.
>
> $ cat void_return.cocci
> @@
> identifier func;
> expression S;
> @@
> void func(...) {
> ...
>         if (...)
> -               return S;
> +               { S; return; }
> ...
> }
>
> @@
> identifier func;
> expression S;
> @@
> void func(...) {
> ...
>         if (...) {
> ...
> -               return S;
> +               S;
>         }
> ...
> }
>
> @@
> identifier func;
> expression S;
> @@
> void func(...) {
> ...
> -       return S;
> +       S;
> }
> $
>
> $ ~/coccinelle/spatch --version
> spatch version 1.0.0-rc20 without Python support and with PCRE support
>
> $ ~/coccinelle/spatch -sp-file void_return.cocci drivers/gpu/drm/i915/i915_irq.c
> init_defs_builtins: /usr/local/share/coccinelle/standard.h
> HANDLING: drivers/gpu/drm/i915/i915_irq.c
> diff =
> --- drivers/gpu/drm/i915/i915_irq.c
> +++ /tmp/cocci-output-13176-0a96c4-i915_irq.c
> @@ -2818,7 +2818,10 @@ static void i915_hangcheck_elapsed(unsig
>         }
>
>         if (rings_hung)
> -               return i915_handle_error(dev, true, "Ring hung");
> +       {
> +               i915_handle_error(dev, true, "Ring hung");
> +               return;
> +       }
>
>         if (busy_count)
>                 /* Reset timer case chip hangs without another request
>
>
> and with an older spatch version
>
> $ spatch --version
> spatch version 1.0.0-rc14 without Python support and with PCRE support
>
> $ spatch -sp-file void_return.cocci drivers/gpu/drm/i915/i915_irq.c
> init_defs_builtins: /usr/local/share/coccinelle/standard.h
> HANDLING: drivers/gpu/drm/i915/i915_irq.c
> diff =
> --- drivers/gpu/drm/i915/i915_irq.c
> +++ /tmp/cocci-output-13258-bc64a5-i915_irq.c
> @@ -2817,8 +2817,11 @@ static void i915_hangcheck_elapsed(unsig
>                 }
>         }
>
> -       if (rings_hung)
> -               return i915_handle_error(dev, true, "Ring hung");
> +       if (rings_hung) {
> +
> +                       i915_handle_error(dev, true, "Ring hung");
> +                       return;
> +               }
>
>         if (busy_count)
>                 /* Reset timer case chip hangs without another request
>
>
> _______________________________________________
> Cocci mailing list
> Cocci at systeme.lip6.fr
> https://systeme.lip6.fr/mailman/listinfo/cocci



-- 
Peter

[Cocci] "trivial" linux code style reformatting

[SF Markus Elfring]

> I tried this little cocci script to remove unnecessary
> uses of return in void functions.

I find that this description does not fit to the first SmPL rule. I would
interpret it in the way that curly braces should eventually be added.

Do you need to specify any further SmPL constraints for the source code which
might follow "an unwanted return statement"?

Regards,
Markus

[Cocci] coccinelle: trivial linux code style reformatting

[Joe Perches]

On Tue, 2014-04-01 at 20:29 +0200, Peter Senna Tschudin wrote:
> I changed the first rule to:
> 
> @@
> identifier func;
> expression S,E;
> @@
> void func(...) {
> ...
> - if (E) return S;
> + if (E) {S; return;}

Thanks Peter, that makes more sense than
what I wrote.

cheers, Joe

[Cocci] "trivial" linux code style reformatting

[Joe Perches]

On Tue, 2014-04-01 at 22:02 +0200, SF Markus Elfring wrote:
> > I tried this little cocci script to remove unnecessary
> > uses of return in void functions.
> 
> I find that this description does not fit to the first SmPL rule. I would
> interpret it in the way that curly braces should eventually be added.

Yes, they should be added.

> Do you need to specify any further SmPL constraints for the source code which
> might follow "an unwanted return statement"?

That's what the ... statements are for and
why there are 3 transforms.

[Cocci] coccinelle: trivial linux code style reformatting

[Julia Lawall]

On Tue, 1 Apr 2014, Joe Perches wrote:

> On Tue, 2014-04-01 at 20:29 +0200, Peter Senna Tschudin wrote:
> > I changed the first rule to:
> > 
> > @@
> > identifier func;
> > expression S,E;
> > @@
> > void func(...) {
> > ...
> > - if (E) return S;
> > + if (E) {S; return;}
> 
> Thanks Peter, that makes more sense than
> what I wrote.

I don't think it makes any more sense.  Indeed, it works mostly better, 
because Coccinelle takes charge of the interaction between the ) and the 
{, but if ther is some specific spacing in E, that will be lost.  I think 
that version rc20 should have handled the original rule correctly.  I will 
look into it.

On the other hand, I didn't understand the second rule, 

> @@
> identifier func;
> expression S;
> @@
> void func(...) {
> ...
>         if (...) {
> ...
> -               return S;
> +               S;
>         }
> ...
> }

at all.  Should it be + S; return;?

Finally, in both cases, I guess you want the rule to apply anywhere it 
occurs.  So eg the first rule should be:

void func(...) {
<...
- if (E) return S;
+ if (E) {S; return;}
...>
}

julia

[Cocci] coccinelle: trivial linux code style reformatting

[Joe Perches]

On Wed, 2014-04-02 at 07:53 +0200, Julia Lawall wrote:
> On Tue, 1 Apr 2014, Joe Perches wrote:
> > On Tue, 2014-04-01 at 20:29 +0200, Peter Senna Tschudin wrote:
> > > I changed the first rule to:
> > > 
> > > @@
> > > identifier func;
> > > expression S,E;
> > > @@
> > > void func(...) {
> > > ...
> > > - if (E) return S;
> > > + if (E) {S; return;}
> > 
> > Thanks Peter, that makes more sense than
> > what I wrote.
> 
> I don't think it makes any more sense.  Indeed, it works mostly better, 
> because Coccinelle takes charge of the interaction between the ) and the 
> {, but if ther is some specific spacing in E, that will be lost.  I think 
> that version rc20 should have handled the original rule correctly.  I will 
> look into it.

Thanks.

> On the other hand, I didn't understand the second rule, 

It's broken.

> > @@
> > identifier func;
> > expression S;
> > @@
> > void func(...) {
> > ...
> >         if (...) {
> > ...
> > -               return S;
> > +               S;
> >         }
> > ...
> > }
> 
> at all.  Should it be + S; return;?

Yes, true, just like the first case.

[Cocci] Clarification for source code style reformatting

[SF Markus Elfring]

> The output is apparently correct and compilable.
> It is just not kernel style.  I'm not sure there
> is a defect really, just wanted to bring it up.
> 
> Maybe the first transform could be better written.

It seems that the shown differences in source code formatting come from your
first semantic patch rule.
Does your update suggestion fit to implementations of functions which had a
non-void return type before?


> $ cat void_return.cocci
> @@
> identifier func;
> expression S;
> @@
> void func(...) {
> ...
> 	if (...)
> -		return S;
> +		{ S; return; }
> ...
> }

I have got another view for further consideration around similar SmPL approaches.

You expect that the adjusted source code should fit to the Linux coding guidelines.
https://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/tree/Documentation/CodingStyle?id=b57a0505e750b3d7b2d39e9823b276d8ca1a08fe

I guess that would mean here that the opening curly brace should be placed at
the end of the "if line".
But: Does your SmPL approach express a desire to leave this source code line
unchanged?

Does the behaviour from the tool "spatch 1.0.0-rc14" come closer to your
expectations for this use case?
Does your example show an interesting target conflict?

Various software developers have got strong opinions about the recommended
source code formatting.
http://en.wikipedia.org/wiki/Programming_style

Is there a need to make the "pretty-printing strategy" configurable for the
Coccinelle software?
http://en.wikipedia.org/wiki/Prettyprint#Programming_code_formatting_and_beautification

Regards,
Markus

[Cocci] Clarification for source code style reformatting

[Julia Lawall]

On Wed, 2 Apr 2014, SF Markus Elfring wrote:

> > The output is apparently correct and compilable.
> > It is just not kernel style.  I'm not sure there
> > is a defect really, just wanted to bring it up.
> >
> > Maybe the first transform could be better written.
>
> It seems that the shown differences in source code formatting come from your
> first semantic patch rule.
> Does your update suggestion fit to implementations of functions which had a
> non-void return type before?

The transformation makes no sense for a non-void return type.

The intent of Coccinelle is that the output should follow the Linux coding
guidelines.  The pretty printing, however, is fairly fragile and it looks
like this case has slightly degraded in some way, although the previous
generated code was not ideal either, due to the introduced newline.

julia

[Cocci] Clarification for source code style reformatting

[SF Markus Elfring]

> The intent of Coccinelle is that the output should follow the Linux coding guidelines.

I would find it also useful to support other source code format styles.
How do you think about corresponding configuration options for the
pretty-printing functionality?

Regards,
Markus

[Cocci] Clarification for source code style reformatting

[Joe Perches]

On Wed, 2014-04-02 at 09:19 +0200, SF Markus Elfring wrote:
> > The output is apparently correct and compilable.
> > It is just not kernel style.  I'm not sure there
> > is a defect really, just wanted to bring it up.
> > 
> > Maybe the first transform could be better written.
> 
> It seems that the shown differences in source code formatting come from your
> first semantic patch rule.
> Does your update suggestion fit to implementations of functions which had a
> non-void return type before?

void functions cannot have non-void returns.

> > $ cat void_return.cocci
> > @@
> > identifier func;
> > expression S;
> > @@
> > void func(...) {

This test is only for void functions.

[Cocci] "trivial" linux code style reformatting

[SF Markus Elfring]

> Indeed, it works mostly better, because Coccinelle takes charge
> of the interaction between the ) and the {, but if ther is some
> specific spacing in E, that will be lost.

Would you like to clarify this implementation detail a bit more?
Are there any chances that this software will become able to restore a preferred
source code layout from the matched expressions?

Regards,
Markus

[Cocci] Clarification for source code style reformatting

[Julia Lawall]

On Wed, 2 Apr 2014, SF Markus Elfring wrote:

> > The intent of Coccinelle is that the output should follow the Linux coding guidelines.
>
> I would find it also useful to support other source code format styles.
> How do you think about corresponding configuration options for the
> pretty-printing functionality?

Sorry, I forgot to add that there is an option --smpl-spacing where the
result follows the format found in the semantic patch.

julia

[Cocci] "trivial" linux code style reformatting

[Julia Lawall]

On Wed, 2 Apr 2014, SF Markus Elfring wrote:

> > Indeed, it works mostly better, because Coccinelle takes charge
> > of the interaction between the ) and the {, but if ther is some
> > specific spacing in E, that will be lost.
>
> Would you like to clarify this implementation detail a bit more?
> Are there any chances that this software will become able to restore a preferred
> source code layout from the matched expressions?

No, a metavariable contains no information about its format, so that it
can match with other metavariables, and when it is printed, Coccinelle
follows whatever strategy it follows, which is intended to follow the
conventions of Linux, but is not always successful.  There are tools such
as indent that reformat software as a whole using various conventions.

julia

[Cocci] code style reformatting

[SF Markus Elfring]

>> Are there any chances that this software will become able to restore a preferred
>> source code layout from the matched expressions?
> 
> No, a metavariable contains no information about its format, so that it
> can match with other metavariables, and when it is printed, Coccinelle
> follows whatever strategy it follows, which is intended to follow the
> conventions of Linux, but is not always successful.

Thanks for this clarification.


> There are tools such as indent that reformat software as a whole using
> various conventions.

How do you think about to avoid such additional source code style reformatting?
Can alternative layout rules be integrated into an improved handling of semantic
patches?

Regards,
Markus

[Cocci] Restore source code from matched expressions?

[SF Markus Elfring]

>> Are there any chances that this software will become able to restore a preferred
>> source code layout from the matched expressions?
> 
> No, a metavariable contains no information about its format, so that it
> can match with other metavariables, and when it is printed, Coccinelle
> follows whatever strategy it follows, which is intended to follow the
> conventions of Linux, but is not always successful.

Do SmPL metavariables also manage the start and end positions for the source
code part which was matched in a specific analysis context?
Is such information not only available for position metavariables?

Regards,
Markus