[PATCH] coccinelle: if(ret)return ret; return ret; semantic patch

[PATCH] coccinelle: if(ret)return ret; return ret; semantic patch

[Greg Dietsche]

This semantic patch finds code matching this pattern:
	if(ret)
		return ret;
	return ret;

I will be submitting patches shortly against the mainline to cleanup all
code matching this pattern.

Signed-off-by: Greg Dietsche <Gregory.Dietsche@cuw.edu>
---
 scripts/coccinelle/misc/doublereturn.cocci |   20 ++++++++++++++++++++
 1 files changed, 20 insertions(+), 0 deletions(-)
 create mode 100644 scripts/coccinelle/misc/doublereturn.cocci

diff --git a/scripts/coccinelle/misc/doublereturn.cocci b/scripts/coccinelle/misc/doublereturn.cocci
new file mode 100644
index 0000000..656a118
--- /dev/null
+++ b/scripts/coccinelle/misc/doublereturn.cocci
@@ -0,0 +1,20 @@
+/// Remove unecessary if/return in code that follows this pattern:
+///	if(retval)
+///		return retval;
+///	return retval;
+//
+// Confidence: High
+// Copyright: (C) 2011 Greg Dietsche GPLv2.
+// URL: http://www.gregd.org
+// Comments:
+// Options: -no_includes
+
+virtual patch
+
+@@
+identifier retval;
+@@
+-if (retval)
+-	return retval;
+-return retval;
++return retval;
-- 
1.7.2.5

Re: [PATCH] coccinelle: if(ret)return ret; return ret; semantic patch

[Julia Lawall]

Thanks.  I tried this too, but I wasn't sure about the results.  The 
question is why stop here.  For example, there are IS_ERR calls that one 
could consider as well.  Or ret < 0.  Or why not just:

@@
expression ret;
@@

- if (...) return ret;
  return ret;

Although there might be function calls that one doesn't want to touch, so:

@@
identifier f != IS_ERR;
expression ret;
statement S;
@@

(
if (<+...f(...)...+>) S
|
- if (...) return ret;
  return ret;
)

julia


On Sun, 5 Jun 2011, Greg Dietsche wrote:

> This semantic patch finds code matching this pattern:
> 	if(ret)
> 		return ret;
> 	return ret;
> 
> I will be submitting patches shortly against the mainline to cleanup all
> code matching this pattern.
> 
> Signed-off-by: Greg Dietsche <Gregory.Dietsche@cuw.edu>
> ---
>  scripts/coccinelle/misc/doublereturn.cocci |   20 ++++++++++++++++++++
>  1 files changed, 20 insertions(+), 0 deletions(-)
>  create mode 100644 scripts/coccinelle/misc/doublereturn.cocci
> 
> diff --git a/scripts/coccinelle/misc/doublereturn.cocci b/scripts/coccinelle/misc/doublereturn.cocci
> new file mode 100644
> index 0000000..656a118
> --- /dev/null
> +++ b/scripts/coccinelle/misc/doublereturn.cocci
> @@ -0,0 +1,20 @@
> +/// Remove unecessary if/return in code that follows this pattern:
> +///	if(retval)
> +///		return retval;
> +///	return retval;
> +//
> +// Confidence: High
> +// Copyright: (C) 2011 Greg Dietsche GPLv2.
> +// URL: http://www.gregd.org
> +// Comments:
> +// Options: -no_includes
> +
> +virtual patch
> +
> +@@
> +identifier retval;
> +@@
> +-if (retval)
> +-	return retval;
> +-return retval;
> ++return retval;
> -- 
> 1.7.2.5
> 
> 

Re: [PATCH] coccinelle: if(ret)return ret; return ret; semantic patch

[Julia Lawall]

On Sun, 5 Jun 2011, Greg Dietsche wrote:

> This semantic patch finds code matching this pattern:
> 	if(ret)
> 		return ret;
> 	return ret;
> 
> I will be submitting patches shortly against the mainline to cleanup all
> code matching this pattern.
> 
> Signed-off-by: Greg Dietsche <Gregory.Dietsche@cuw.edu>
> ---
>  scripts/coccinelle/misc/doublereturn.cocci |   20 ++++++++++++++++++++
>  1 files changed, 20 insertions(+), 0 deletions(-)
>  create mode 100644 scripts/coccinelle/misc/doublereturn.cocci
> 
> diff --git a/scripts/coccinelle/misc/doublereturn.cocci b/scripts/coccinelle/misc/doublereturn.cocci
> new file mode 100644
> index 0000000..656a118
> --- /dev/null
> +++ b/scripts/coccinelle/misc/doublereturn.cocci
> @@ -0,0 +1,20 @@
> +/// Remove unecessary if/return in code that follows this pattern:
> +///	if(retval)
> +///		return retval;
> +///	return retval;
> +//
> +// Confidence: High
> +// Copyright: (C) 2011 Greg Dietsche GPLv2.
> +// URL: http://www.gregd.org
> +// Comments:
> +// Options: -no_includes
> +
> +virtual patch
> +
> +@@

This line should be @depends on patch@

julia

> +identifier retval;
> +@@
> +-if (retval)
> +-	return retval;
> +-return retval;
> ++return retval;
> -- 
> 1.7.2.5
> 
> 

Re: [PATCH] coccinelle: if(ret)return ret; return ret; semantic patch

[Greg Dietsche]

Hi Julia,

On 06/05/2011 11:55 PM, Julia Lawall wrote:
> Thanks.  I tried this too, but I wasn't sure about the results.  The
> question is why stop here.
very interesting! Honestly I hadn't thought much further than what you 
see in my semantic patch. I'd noticed piece of code in the iwlegacy 
driver and wondered what'd happen if I taught myself a little bit about 
cocci :)
>    For example, there are IS_ERR calls that one
> could consider as well.  Or ret<  0.  Or why not just:
>
> @@
> expression ret;
> @@
>
> - if (...) return ret;
>    return ret;
>
> Although there might be function calls that one doesn't want to touch, so:
>
> @@
> identifier f != IS_ERR;
> expression ret;
> statement S;
> @@
>
> (
> if (<+...f(...)...+>) S
> |
> - if (...) return ret;
>    return ret;
> )
>
> julia
>
>
>    
There seem to be many variations on the theme to consider... though 
hopefully the compiler optimizes most of these out... For example, in 
sound/soc/codecs/wm8940.c, Jonathan Cameron pointed some code out to me 
that looks like this:

         if (ret)
                 goto error_ret;

error_ret:
         return ret;




As an aside, I added a feature to the script for myself so that I can 
for example write 'make coccicheck M=drivers/net/wireless/' for example 
to focus in on that directory and just run the checks there... I can 
submit a patch for this... though I was wondering if there is already a 
way to do this and I just missed it. The thought was to make it work the 
same way you'd build a module.


Greg

Re: [PATCH] coccinelle: if(ret)return ret; return ret; semantic patch

[Julia Lawall]

On Tue, 7 Jun 2011, Greg Dietsche wrote:

> Hi Julia,
> 
> On 06/05/2011 11:55 PM, Julia Lawall wrote:
> > Thanks.  I tried this too, but I wasn't sure about the results.  The
> > question is why stop here.
> very interesting! Honestly I hadn't thought much further than what you see in
> my semantic patch. I'd noticed piece of code in the iwlegacy driver and
> wondered what'd happen if I taught myself a little bit about cocci :)
> >    For example, there are IS_ERR calls that one
> > could consider as well.  Or ret<  0.  Or why not just:
> >
> > @@
> > expression ret;
> > @@
> >
> > - if (...) return ret;
> >    return ret;
> >
> > Although there might be function calls that one doesn't want to touch, so:
> >
> > @@
> > identifier f != IS_ERR;
> > expression ret;
> > statement S;
> > @@
> >
> > (
> > if (<+...f(...)...+>) S
> > |
> > - if (...) return ret;
> >    return ret;
> > )
> >
> > julia
> >
> >
> >    
> There seem to be many variations on the theme to consider... though hopefully
> the compiler optimizes most of these out... For example, in
> sound/soc/codecs/wm8940.c, Jonathan Cameron pointed some code out to me that
> looks like this:
> 
>         if (ret)
>                 goto error_ret;
> 
> error_ret:
>         return ret;

Good one :)

> As an aside, I added a feature to the script for myself so that I can for
> example write 'make coccicheck M=drivers/net/wireless/' for example to focus
> in on that directory and just run the checks there... I can submit a patch for
> this... though I was wondering if there is already a way to do this and I just
> missed it. The thought was to make it work the same way you'd build a module.

I think it is possible, but Nicolas would know better.

julia

Re: [PATCH] coccinelle: if(ret)return ret; return ret; semantic patch

[Nicolas Palix]

Hi,

On Tue, Jun 7, 2011 at 6:56 PM, Julia Lawall <julia@diku.dk> wrote:
>> As an aside, I added a feature to the script for myself so that I can for
>> example write 'make coccicheck M=drivers/net/wireless/' for example to focus
>> in on that directory and just run the checks there... I can submit a patch for
>> this... though I was wondering if there is already a way to do this and I just
>> missed it. The thought was to make it work the same way you'd build a module.
>
> I think it is possible, but Nicolas would know better.
>

I remember me having worked on that but I cannot find any patch
about it so your patch is more that welcome. Some users have requested it
some months ago.

As far as I can remember, with "M=" you can not use $srctree only but still
need it for the -I for instance. I think I then run into a situation where I
have an ambiguity about how to setup the flags for coccinelle, but I am sure
about the reason I dropped my patch. Maybe I don't know well enough the build
system and options...

So, don't hesitate to submit your patch.
Does it also update the Documentation/coccinelle.txt accordingly ?

-- 
Nicolas Palix
http://sardes.inrialpes.fr/~npalix/

Re: [PATCH] coccinelle: if(ret)return ret; return ret; semantic patch

[Greg Dietsche]

On 06/07/2011 04:54 PM, Nicolas Palix wrote:
> I remember me having worked on that but I cannot find any patch
> about it so your patch is more that welcome. Some users have requested it
> some months ago.
>
> As far as I can remember, with "M=" you can not use $srctree only but still
> need it for the -I for instance. I think I then run into a situation where I
> have an ambiguity about how to setup the flags for coccinelle, but I am sure
> about the reason I dropped my patch. Maybe I don't know well enough the build
> system and options...
>
> So, don't hesitate to submit your patch.
>    
I'll submit what I have done as a new thread / patch series. I've not 
done much testing with it and am certainly not any kind of build system 
expert, but it has worked well for me so far :) A few more people trying 
it out / testing it would be a great idea.
> Does it also update the Documentation/coccinelle.txt accordingly ?
>    
It will :)

Greg

Re: [PATCH] coccinelle: if(ret)return ret; return ret; semantic patch

[Nicolas Palix]

Hi,

On Wed, Jun 8, 2011 at 12:59 AM, Greg Dietsche <gregory.dietsche@cuw.edu> wrote:
> On 06/07/2011 04:54 PM, Nicolas Palix wrote:
>>
>> I remember me having worked on that but I cannot find any patch
>> about it so your patch is more that welcome. Some users have requested it
>> some months ago.
>>
>> As far as I can remember, with "M=" you can not use $srctree only but
>> still
>> need it for the -I for instance. I think I then run into a situation where
>> I
>> have an ambiguity about how to setup the flags for coccinelle, but I am
>> sure
>> about the reason I dropped my patch. Maybe I don't know well enough the
>> build
>> system and options...
>>
>> So, don't hesitate to submit your patch.
>>
>
> I'll submit what I have done as a new thread / patch series. I've not done
> much testing with it and am certainly not any kind of build system expert,
> but it has worked well for me so far :) A few more people trying it out /
> testing it would be a great idea.

I finally found the thread where we discuss this feature.
http://lkml.org/lkml/2010/7/2/279

I haven't the time to test your patch right now but at the time
the problem was in the patch mode which produces erroneous file names
(in the lines with ---/+++).
As far as I can remember, using the -dir and -patch options of
coccinelle doesn't work with external modules.



>>
>> Does it also update the Documentation/coccinelle.txt accordingly ?
>>
>
> It will :)
>
> Greg
>
>



-- 
Nicolas Palix
http://sardes.inrialpes.fr/~npalix/

Re: [PATCH] coccinelle: if(ret)return ret; return ret; semantic patch

[Julia Lawall]

On Wed, 8 Jun 2011, Nicolas Palix wrote:

> Hi,
> 
> On Wed, Jun 8, 2011 at 12:59 AM, Greg Dietsche <gregory.dietsche@cuw.edu> wrote:
> > On 06/07/2011 04:54 PM, Nicolas Palix wrote:
> >>
> >> I remember me having worked on that but I cannot find any patch
> >> about it so your patch is more that welcome. Some users have requested it
> >> some months ago.
> >>
> >> As far as I can remember, with "M=" you can not use $srctree only but
> >> still
> >> need it for the -I for instance. I think I then run into a situation where
> >> I
> >> have an ambiguity about how to setup the flags for coccinelle, but I am
> >> sure
> >> about the reason I dropped my patch. Maybe I don't know well enough the
> >> build
> >> system and options...
> >>
> >> So, don't hesitate to submit your patch.
> >>
> >
> > I'll submit what I have done as a new thread / patch series. I've not done
> > much testing with it and am certainly not any kind of build system expert,
> > but it has worked well for me so far :) A few more people trying it out /
> > testing it would be a great idea.
> 
> I finally found the thread where we discuss this feature.
> http://lkml.org/lkml/2010/7/2/279
> 
> I haven't the time to test your patch right now but at the time
> the problem was in the patch mode which produces erroneous file names
> (in the lines with ---/+++).
> As far as I can remember, using the -dir and -patch options of
> coccinelle doesn't work with external modules.

Can you give a concrete example?  -patch is supposed to address the 
problem of undesired path elements, but perhaps it is not sufficient in 
this case.  There could be a problem of knowing what the argument to 
-patch should be?

julia 

Re: [PATCH] coccinelle: if(ret)return ret; return ret; semantic patch

[Greg Dietsche]

On 06/05/2011 11:55 PM, Julia Lawall wrote:
> Thanks.  I tried this too, but I wasn't sure about the results.  The
> question is why stop here.  For example, there are IS_ERR calls that one
> could consider as well.  Or ret<  0.  Or why not just:
>
> @@
> expression ret;
> @@
>
> - if (...) return ret;
>    return ret;
>
>    
I've been thinking a bit more about this. Is there a community 
preference towards patches that are highly reliable v.s. ones that find 
things that might not be a problem? I'm leaning towards updating my 
patch to do the above and then later coming back when I have more time 
to fix it up to find only things that are really problems.

Greg
> Although there might be function calls that one doesn't want to touch, so:
>
> @@
> identifier f != IS_ERR;
> expression ret;
> statement S;
> @@
>
> (
> if (<+...f(...)...+>) S
> |
> - if (...) return ret;
>    return ret;
> )
>
> julia
>
>
> On Sun, 5 Jun 2011, Greg Dietsche wrote:
>
>    
>> This semantic patch finds code matching this pattern:
>> 	if(ret)
>> 		return ret;
>> 	return ret;
>>
>> I will be submitting patches shortly against the mainline to cleanup all
>> code matching this pattern.
>>
>> Signed-off-by: Greg Dietsche<Gregory.Dietsche@cuw.edu>
>> ---
>>   scripts/coccinelle/misc/doublereturn.cocci |   20 ++++++++++++++++++++
>>   1 files changed, 20 insertions(+), 0 deletions(-)
>>   create mode 100644 scripts/coccinelle/misc/doublereturn.cocci
>>
>> diff --git a/scripts/coccinelle/misc/doublereturn.cocci b/scripts/coccinelle/misc/doublereturn.cocci
>> new file mode 100644
>> index 0000000..656a118
>> --- /dev/null
>> +++ b/scripts/coccinelle/misc/doublereturn.cocci
>> @@ -0,0 +1,20 @@
>> +/// Remove unecessary if/return in code that follows this pattern:
>> +///	if(retval)
>> +///		return retval;
>> +///	return retval;
>> +//
>> +// Confidence: High
>> +// Copyright: (C) 2011 Greg Dietsche GPLv2.
>> +// URL: http://www.gregd.org
>> +// Comments:
>> +// Options: -no_includes
>> +
>> +virtual patch
>> +
>> +@@
>> +identifier retval;
>> +@@
>> +-if (retval)
>> +-	return retval;
>> +-return retval;
>> ++return retval;
>> -- 
>> 1.7.2.5
>>
>>
>>      
>    

Re: [PATCH] coccinelle: if(ret)return ret; return ret; semantic patch

[Julia Lawall]

On Sat, 11 Jun 2011, Greg Dietsche wrote:

> On 06/05/2011 11:55 PM, Julia Lawall wrote:
> > Thanks.  I tried this too, but I wasn't sure about the results.  The
> > question is why stop here.  For example, there are IS_ERR calls that one
> > could consider as well.  Or ret<  0.  Or why not just:
> >
> > @@
> > expression ret;
> > @@
> >
> > - if (...) return ret;
> >    return ret;
> >
> >    
> I've been thinking a bit more about this. Is there a community preference
> towards patches that are highly reliable v.s. ones that find things that might
> not be a problem? I'm leaning towards updating my patch to do the above and
> then later coming back when I have more time to fix it up to find only things
> that are really problems.

I tend to write the patch that returns the kinds of thing that I know how 
to fix :).  That is, if I see a lot of reports that I don't know what to 
do with, I write the semantic patch to avoid showing them to me.

When you put your patch in the kernel, you can put some comments at the 
beginning of it that say your confidence in the results and give some 
hints about possible false positives.

Overall, I think you should just use your judgement :)

thanks,
julia

> Greg
> > Although there might be function calls that one doesn't want to touch, so:
> >
> > @@
> > identifier f != IS_ERR;
> > expression ret;
> > statement S;
> > @@
> >
> > (
> > if (<+...f(...)...+>) S
> > |
> > - if (...) return ret;
> >    return ret;
> > )
> >
> > julia
> >
> >
> > On Sun, 5 Jun 2011, Greg Dietsche wrote:
> >
> >    
> > > This semantic patch finds code matching this pattern:
> > >  if(ret)
> > >  	return ret;
> > >  return ret;
> > >
> > > I will be submitting patches shortly against the mainline to cleanup all
> > > code matching this pattern.
> > >
> > > Signed-off-by: Greg Dietsche<Gregory.Dietsche@cuw.edu>
> > > ---
> > >   scripts/coccinelle/misc/doublereturn.cocci |   20 ++++++++++++++++++++
> > >   1 files changed, 20 insertions(+), 0 deletions(-)
> > >   create mode 100644 scripts/coccinelle/misc/doublereturn.cocci
> > >
> > > diff --git a/scripts/coccinelle/misc/doublereturn.cocci
> > > b/scripts/coccinelle/misc/doublereturn.cocci
> > > new file mode 100644
> > > index 0000000..656a118
> > > --- /dev/null
> > > +++ b/scripts/coccinelle/misc/doublereturn.cocci
> >> @@ -0,0 +1,20 @@
> > > +/// Remove unecessary if/return in code that follows this pattern:
> > > +///	if(retval)
> > > +///		return retval;
> > > +///	return retval;
> > > +//
> > > +// Confidence: High
> > > +// Copyright: (C) 2011 Greg Dietsche GPLv2.
> > > +// URL: http://www.gregd.org
> > > +// Comments:
> > > +// Options: -no_includes
> > > +
> > > +virtual patch
> > > +
> > > +@@
> > > +identifier retval;
> > > +@@
> > > +-if (retval)
> > > +-	return retval;
> > > +-return retval;
> > > ++return retval;
> > > -- 
> > > 1.7.2.5
> > >
> > >
> > >      
> >    
> 

[PATCH v2] coccinelle: if (ret) return ret; return ret; semantic patch

[Greg Dietsche]

Semantic patch to find code that matches this pattern:
	if (...) return ret;
	return ret;

Version 2:
	Incorporate review comments from Julia Lawall
	Add matches for a number of other similar patterns.

Signed-off-by: Greg Dietsche <Gregory.Dietsche@cuw.edu>
---
 scripts/coccinelle/misc/doublereturn.cocci |   34 ++++++++++++++++++++++++++++
 1 files changed, 34 insertions(+), 0 deletions(-)
 create mode 100644 scripts/coccinelle/misc/doublereturn.cocci

diff --git a/scripts/coccinelle/misc/doublereturn.cocci b/scripts/coccinelle/misc/doublereturn.cocci
new file mode 100644
index 0000000..b41e6f2
--- /dev/null
+++ b/scripts/coccinelle/misc/doublereturn.cocci
@@ -0,0 +1,34 @@
+// Removing unecessary code that matches this core pattern:
+//	-if(...) return ret;
+//	return ret;
+//
+// Confidence: High
+// Copyright: (C) 2011 Greg Dietsche GPLv2.
+// URL: http://www.gregd.org
+// Comments:
+// Options: -no_includes
+
+virtual patch
+
+@depends on patch@
+expression ret, operand;
+identifier is_ordinal_table ~= "IS_ORDINAL_TABLE_\(ONE\|TWO\)";
+@@
+(
+//via an isomorphism this also covers ret and unlikely(ret)
+-if (likely(ret)) return ret;
+|
+-if (IS_ZERO(ret)) return ret;
+|
+-if (is_ordinal_table(...)) return ret;
+|
+-if (!ret) return ret;
+|
+-if (ret > operand) return ret;
+|
+-if (ret < operand) return ret;
+|
+-if (ret != operand) return ret;
+)
+-return ret;
++return ret;
-- 
1.7.2.5

Re: [PATCH v2] coccinelle: if (ret) return ret; return ret; semantic patch

[Joe Perches]

On Mon, 2011-06-13 at 13:23 -0500, Greg Dietsche wrote:
> Semantic patch to find code that matches this pattern:
[]
> diff --git a/scripts/coccinelle/misc/doublereturn.cocci b/scripts/coccinelle/misc/doublereturn.cocci
[]
> +-if (ret > operand) return ret;
> +|
> +-if (ret < operand) return ret;

Maybe add <= and >=

Re: [PATCH v2] coccinelle: if (ret) return ret; return ret; semantic patch

[Julia Lawall]

On Mon, 13 Jun 2011, Greg Dietsche wrote:

> Semantic patch to find code that matches this pattern:
> 	if (...) return ret;
> 	return ret;
> 
> Version 2:
> 	Incorporate review comments from Julia Lawall
> 	Add matches for a number of other similar patterns.
> 
> Signed-off-by: Greg Dietsche <Gregory.Dietsche@cuw.edu>
> ---
>  scripts/coccinelle/misc/doublereturn.cocci |   34 ++++++++++++++++++++++++++++
>  1 files changed, 34 insertions(+), 0 deletions(-)
>  create mode 100644 scripts/coccinelle/misc/doublereturn.cocci
> 
> diff --git a/scripts/coccinelle/misc/doublereturn.cocci b/scripts/coccinelle/misc/doublereturn.cocci
> new file mode 100644
> index 0000000..b41e6f2
> --- /dev/null
> +++ b/scripts/coccinelle/misc/doublereturn.cocci
> @@ -0,0 +1,34 @@
> +// Removing unecessary code that matches this core pattern:
> +//	-if(...) return ret;
> +//	return ret;
> +//
> +// Confidence: High
> +// Copyright: (C) 2011 Greg Dietsche GPLv2.
> +// URL: http://www.gregd.org
> +// Comments:
> +// Options: -no_includes
> +
> +virtual patch
> +
> +@depends on patch@
> +expression ret, operand;
> +identifier is_ordinal_table ~= "IS_ORDINAL_TABLE_\(ONE\|TWO\)";
> +@@
> +(
> +//via an isomorphism this also covers ret and unlikely(ret)
> +-if (likely(ret)) return ret;
> +|
> +-if (IS_ZERO(ret)) return ret;
> +|
> +-if (is_ordinal_table(...)) return ret;
> +|
> +-if (!ret) return ret;
> +|
> +-if (ret > operand) return ret;
> +|
> +-if (ret < operand) return ret;
> +|
> +-if (ret != operand) return ret;
> +)
> +-return ret;
> ++return ret;
> -- 
> 1.7.2.5

How about:

@@
identifier f;
expression ret;
identifier x;
@@

(
- if (likely(x)) return ret;
|
- if (\(IS_ERR\|IS_ZERO\|is_ordinal_table\)(x)) return ret;
|
  if (<+...f(...)...+>) return ret;
|
- if (...) return ret;
)
return ret;

I have put the likely case separate from the other function calls to 
benefit from the isomorphism.  I have restricted the argument to these 
functions to be an identifier so that it won't have any side effects.  It 
doesn't have to be the same as ret though.  The third line keeps all other 
ifs that contain function calls.  The fourth line gets rid of everything 
else.

You could see if this finds all of the cases of your proposed rule and if 
it at least doesn't find anything else that you don't want it to find.

julia

Re: [PATCH v2] coccinelle: if (ret) return ret; return ret; semantic patch

[Greg Dietsche]

On 06/13/2011 01:38 PM, Julia Lawall wrote:
> How about:
>
> @@
> identifier f;
> expression ret;
> identifier x;
> @@
>
> (
> - if (likely(x)) return ret;
> |
> - if (\(IS_ERR\|IS_ZERO\|is_ordinal_table\)(x)) return ret;
> |
>    if (<+...f(...)...+>) return ret;
> |
> - if (...) return ret;
> )
> return ret;
>
>    
just curious... i see you usually just write "return ret;" here when 
posting. I've assumed that's because it will 1) work and 2) is close enough.

You'll notice I've been doing:
-return ret;
+return ret;
because it seems to help coccinelle realize that it can get rid of extra 
line feeds - does this make sense - or should i just be doing a "return 
ret"?
> I have put the likely case separate from the other function calls to
> benefit from the isomorphism.  I have restricted the argument to these
> functions to be an identifier so that it won't have any side effects.  It
> doesn't have to be the same as ret though.  The third line keeps all other
> ifs that contain function calls.  The fourth line gets rid of everything
> else.
>
> You could see if this finds all of the cases of your proposed rule and if
> it at least doesn't find anything else that you don't want it to find.
>
>    
I'll try it out this afternoon/evening hopefully.
> julia
>
>    

There are two other issues with the patch that I've noticed. I'll be 
teaching myself more on coccinelle to figure these out. Unless someone 
else wants to jump in :) So far I've read or skimmed a number of paper's 
that have been written on Coccinelle... I find it all very interesting :)

1) sometimes you see this type of code - which i've chosen to ignore for 
now:
if ((ret=XXXXX) < 0)
     return ret;
return ret;

which could just be simplified to:
return XXXXX;

for an example see the function load_firmware in 
sound/pci/echoaudio/echoaudio_dsp.c

2) after the semantic patch has removed an "if (...)return ret;" Quite 
often, but not always, we end up with this:
ret=...;
return ret;

which of course could just become
return ...;


So as you can see the problems are quite similar, but a little different.

Greg

Re: [PATCH v2] coccinelle: if (ret) return ret; return ret; semantic patch

[Julia Lawall]

On Mon, 13 Jun 2011, Greg Dietsche wrote:

> On 06/13/2011 01:38 PM, Julia Lawall wrote:
> > How about:
> >
> > @@
> > identifier f;
> > expression ret;
> > identifier x;
> > @@
> >
> > (
> > - if (likely(x)) return ret;
> > |
> > - if (\(IS_ERR\|IS_ZERO\|is_ordinal_table\)(x)) return ret;
> > |
> >    if (<+...f(...)...+>) return ret;
> > |
> > - if (...) return ret;
> > )
> > return ret;
> >
> >    
> just curious... i see you usually just write "return ret;" here when posting.
> I've assumed that's because it will 1) work and 2) is close enough.
> 
> You'll notice I've been doing:
> -return ret;
> +return ret;
> because it seems to help coccinelle realize that it can get rid of extra line
> feeds - does this make sense - or should i just be doing a "return ret"?

I wondered why you were doing that :)

Is the problem that the removed if is being replaced by a blank line?  If 
so, that is not intentional.  I will look into it.  If it doesn't happen 
always, an example where it does happen could be helpful.

The disadvantage of removing something and then adding it back is that 
then Coccinelle takes over the pretty printing of that thing.  Since ret 
is usually a variable, it doesn't matter, and since Coccinelle tries to 
follow Linux spacing conventions it might not matter either.  But eg 
f(a,b,c,d) would become f(a, b, c, d).

julia

> > I have put the likely case separate from the other function calls to
> > benefit from the isomorphism.  I have restricted the argument to these
> > functions to be an identifier so that it won't have any side effects.  It
> > doesn't have to be the same as ret though.  The third line keeps all other
> > ifs that contain function calls.  The fourth line gets rid of everything
> > else.
> >
> > You could see if this finds all of the cases of your proposed rule and if
> > it at least doesn't find anything else that you don't want it to find.
> >
> >    
> I'll try it out this afternoon/evening hopefully.
> > julia
> >
> >    
> 
> There are two other issues with the patch that I've noticed. I'll be teaching
> myself more on coccinelle to figure these out. Unless someone else wants to
> jump in :) So far I've read or skimmed a number of paper's that have been
> written on Coccinelle... I find it all very interesting :)
> 
> 1) sometimes you see this type of code - which i've chosen to ignore for now:
> if ((ret=XXXXX) < 0)
>     return ret;
> return ret;
> 
> which could just be simplified to:
> return XXXXX;
> 
> for an example see the function load_firmware in
> sound/pci/echoaudio/echoaudio_dsp.c
> 
> 2) after the semantic patch has removed an "if (...)return ret;" Quite often,
> but not always, we end up with this:
> ret=...;
> return ret;
> 
> which of course could just become
> return ...;
> 
> 
> So as you can see the problems are quite similar, but a little different.
> 
> Greg
> 
> 
> 

Re: [PATCH v2] coccinelle: if (ret) return ret; return ret; semantic patch

[Greg Dietsche]

On 06/14/2011 12:50 AM, Julia Lawall wrote:
> On Mon, 13 Jun 2011, Greg Dietsche wrote:
>    
>> just curious... i see you usually just write "return ret;" here when posting.
>> I've assumed that's because it will 1) work and 2) is close enough.
>>
>> You'll notice I've been doing:
>> -return ret;
>> +return ret;
>> because it seems to help coccinelle realize that it can get rid of extra line
>> feeds - does this make sense - or should i just be doing a "return ret"?
>>      
> I wondered why you were doing that :)
>
> Is the problem that the removed if is being replaced by a blank line?  If
> so, that is not intentional.  I will look into it.  If it doesn't happen
> always, an example where it does happen could be helpful.
>
>    

Some times it gets it right, so it's not always wrong. For example:

diff -u -p a/arch/m68k/math-emu/fp_log.c b/arch/m68k/math-emu/fp_log.c
--- a/arch/m68k/math-emu/fp_log.c 2011-06-13 14:06:37.943954469 -0500
+++ b/arch/m68k/math-emu/fp_log.c 2011-06-14 16:07:22.394954040 -0500
@@ -105,9 +105,6 @@ fp_fetoxm1(struct fp_ext *dest, struct f

         fp_monadic_check(dest, src);

-       if (IS_ZERO(dest))
-               return dest;
-
         return dest;
  }



Here's an example where it got it "wrong" - notice how the blank line is 
missing the - :

diff -u -p a/arch/frv/mm/pgalloc.c b/arch/frv/mm/pgalloc.c
--- a/arch/frv/mm/pgalloc.c 2011-06-13 14:06:37.855954391 -0500
+++ b/arch/frv/mm/pgalloc.c 2011-06-14 16:07:16.714954008 -0500
@@ -136,8 +136,6 @@ pgd_t *pgd_alloc(struct mm_struct *mm)
         pgd_t *pgd;

         pgd = quicklist_alloc(0, GFP_KERNEL, pgd_ctor);
-       if (!pgd)
-               return pgd;

         return pgd;
  }


but when I do
-return ret;
+return ret;

then both of the above examples are "correct"


Greg


> The disadvantage of removing something and then adding it back is that
> then Coccinelle takes over the pretty printing of that thing.  Since ret
> is usually a variable, it doesn't matter, and since Coccinelle tries to
> follow Linux spacing conventions it might not matter either.  But eg
> f(a,b,c,d) would become f(a, b, c, d).
>
> julia
>    

Re: [PATCH v2] coccinelle: if (ret) return ret; return ret; semantic patch

[Greg Dietsche]

On 06/14/2011 04:24 PM, Greg Dietsche wrote:
> On 06/14/2011 12:50 AM, Julia Lawall wrote:
>> On Mon, 13 Jun 2011, Greg Dietsche wrote:
>>> just curious... i see you usually just write "return ret;" here when
>>> posting.
>>> I've assumed that's because it will 1) work and 2) is close enough.
>>>
>>> You'll notice I've been doing:
>>> -return ret;
>>> +return ret;
>>> because it seems to help coccinelle realize that it can get rid of
>>> extra line
>>> feeds - does this make sense - or should i just be doing a "return ret"?
>> I wondered why you were doing that :)
>>
>> Is the problem that the removed if is being replaced by a blank line? If
>> so, that is not intentional. I will look into it. If it doesn't happen
>> always, an example where it does happen could be helpful.
>>
>
> Some times it gets it right, so it's not always wrong. For example:
>
> diff -u -p a/arch/m68k/math-emu/fp_log.c b/arch/m68k/math-emu/fp_log.c
> --- a/arch/m68k/math-emu/fp_log.c 2011-06-13 14:06:37.943954469 -0500
> +++ b/arch/m68k/math-emu/fp_log.c 2011-06-14 16:07:22.394954040 -0500
> @@ -105,9 +105,6 @@ fp_fetoxm1(struct fp_ext *dest, struct f
>
> fp_monadic_check(dest, src);
>
> - if (IS_ZERO(dest))
> - return dest;
> -
> return dest;
> }
>
>
>
> Here's an example where it got it "wrong" - notice how the blank line is
> missing the - :
>
> diff -u -p a/arch/frv/mm/pgalloc.c b/arch/frv/mm/pgalloc.c
> --- a/arch/frv/mm/pgalloc.c 2011-06-13 14:06:37.855954391 -0500
> +++ b/arch/frv/mm/pgalloc.c 2011-06-14 16:07:16.714954008 -0500
> @@ -136,8 +136,6 @@ pgd_t *pgd_alloc(struct mm_struct *mm)
> pgd_t *pgd;
>
> pgd = quicklist_alloc(0, GFP_KERNEL, pgd_ctor);
> - if (!pgd)
> - return pgd;
>
> return pgd;
> }
>
>
> but when I do
> -return ret;
> +return ret;
>
> then both of the above examples are "correct"
>
>
> Greg
>
>
>> The disadvantage of removing something and then adding it back is that
>> then Coccinelle takes over the pretty printing of that thing. Since ret
>> is usually a variable, it doesn't matter, and since Coccinelle tries to
>> follow Linux spacing conventions it might not matter either. But eg
>> f(a,b,c,d) would become f(a, b, c, d).
>>
>> julia
>
>


here is another wierd one.. but this time it adds an extra blank line...:

diff -u -p a/drivers/staging/brcm80211/brcmsmac/phy/phy_cmn.c 
b/drivers/staging/brcm80211/brcmsmac/phy/phy_cmn.c
--- a/drivers/staging/brcm80211/brcmsmac/phy/phy_cmn.c 2011-06-13 
14:06:39.483954235 -0500
+++ b/drivers/staging/brcm80211/brcmsmac/phy/phy_cmn.c 2011-06-14 
19:20:38.915032203 -0500
@@ -3184,8 +3184,7 @@ wlc_user_txpwr_antport_to_rfport(phy_inf
  {
         s8 offset = 0;

-       if (!pi->user_txpwr_at_rfport)
-               return offset;
+
         return offset;
  }

[PATCH v3] coccinelle: if (ret) return ret; return ret; semantic patch

[Greg Dietsche]

Semantic patch to find code that matches this pattern:
	if (...) return ret;
	return ret;

Version 2:
	Incorporate review comments from Julia Lawall
	Add matches for a number of other similar patterns.

Version 3:
	Incorporating more review comments from Julia.

Signed-off-by: Greg Dietsche <Gregory.Dietsche@cuw.edu>
---
 scripts/coccinelle/misc/doublereturn.cocci |   31 ++++++++++++++++++++++++++++
 1 files changed, 31 insertions(+), 0 deletions(-)
 create mode 100644 scripts/coccinelle/misc/doublereturn.cocci

diff --git a/scripts/coccinelle/misc/doublereturn.cocci b/scripts/coccinelle/misc/doublereturn.cocci
new file mode 100644
index 0000000..2de8564
--- /dev/null
+++ b/scripts/coccinelle/misc/doublereturn.cocci
@@ -0,0 +1,31 @@
+// Removing unecessary code that matches this core pattern:
+//	-if(...) return ret;
+//	return ret;
+//
+// Confidence: High
+// Copyright: (C) 2011 Greg Dietsche GPLv2.
+// URL: http://www.gregd.org
+// Comments:
+// Options: -no_includes
+
+virtual patch
+
+@depends on patch@
+expression ret;
+identifier x, y;
+identifier f;
+identifier is_ordinal_table ~= "IS_ORDINAL_TABLE_\(ONE\|TWO\)";
+@@
+(
+//via an isomorphism this also covers x and unlikely(x)
+-if (likely(x)) return ret;
+|
+-if (\(IS_ERR\|IS_ZERO\)(x)) return ret;
+|
+-if (is_ordinal_table(x,y)) return ret;
+|
+if(<+...f(...)...+>) return ret;
+|
+-if (...) return ret;
+)
+return ret;
-- 
1.7.2.5

Re: [PATCH v3] coccinelle: if (ret) return ret; return ret; semantic patch

[Julia Lawall]

On Tue, 14 Jun 2011, Greg Dietsche wrote:

> Semantic patch to find code that matches this pattern:
> 	if (...) return ret;
> 	return ret;
> 
> Version 2:
> 	Incorporate review comments from Julia Lawall
> 	Add matches for a number of other similar patterns.
> 
> Version 3:
> 	Incorporating more review comments from Julia.
> 
> Signed-off-by: Greg Dietsche <Gregory.Dietsche@cuw.edu>
> ---
>  scripts/coccinelle/misc/doublereturn.cocci |   31 ++++++++++++++++++++++++++++
>  1 files changed, 31 insertions(+), 0 deletions(-)
>  create mode 100644 scripts/coccinelle/misc/doublereturn.cocci
> 
> diff --git a/scripts/coccinelle/misc/doublereturn.cocci b/scripts/coccinelle/misc/doublereturn.cocci
> new file mode 100644
> index 0000000..2de8564
> --- /dev/null
> +++ b/scripts/coccinelle/misc/doublereturn.cocci
> @@ -0,0 +1,31 @@
> +// Removing unecessary code that matches this core pattern:
> +//	-if(...) return ret;
> +//	return ret;
> +//
> +// Confidence: High
> +// Copyright: (C) 2011 Greg Dietsche GPLv2.
> +// URL: http://www.gregd.org
> +// Comments:
> +// Options: -no_includes
> +
> +virtual patch
> +
> +@depends on patch@
> +expression ret;
> +identifier x, y;
> +identifier f;
> +identifier is_ordinal_table ~= "IS_ORDINAL_TABLE_\(ONE\|TWO\)";
> +@@
> +(
> +//via an isomorphism this also covers x and unlikely(x)
> +-if (likely(x)) return ret;
> +|
> +-if (\(IS_ERR\|IS_ZERO\)(x)) return ret;
> +|
> +-if (is_ordinal_table(x,y)) return ret;
> +|
> +if(<+...f(...)...+>) return ret;
> +|
> +-if (...) return ret;
> +)
> +return ret;

It doesn't matter in this case, but in general the use of regular 
expressions should be a last resort.  The problem is that when coccinelle 
is searching for relevant files, it doesn't interpret regular expressions, 
and so it would just take all files.  But in this case it doesn't matter, 
because including eg if (...) return ret; will cause it to consider all 
files anyway.

julia

Re: [PATCH v2] coccinelle: if (ret) return ret; return ret; semantic patch

[Julia Lawall]

On Tue, 14 Jun 2011, Greg Dietsche wrote:

> 
> 
> On 06/14/2011 04:24 PM, Greg Dietsche wrote:
> > On 06/14/2011 12:50 AM, Julia Lawall wrote:
> > > On Mon, 13 Jun 2011, Greg Dietsche wrote:
> > > > just curious... i see you usually just write "return ret;" here when
> > > > posting.
> > > > I've assumed that's because it will 1) work and 2) is close enough.
> > > >
> > > > You'll notice I've been doing:
> > > > -return ret;
> > > > +return ret;
> > > > because it seems to help coccinelle realize that it can get rid of
> > > > extra line
> > > > feeds - does this make sense - or should i just be doing a "return ret"?
> > > I wondered why you were doing that :)
> > >
> > > Is the problem that the removed if is being replaced by a blank line? If
> > > so, that is not intentional. I will look into it. If it doesn't happen
> > > always, an example where it does happen could be helpful.
> > >
> >
> > Some times it gets it right, so it's not always wrong. For example:
> >
> > diff -u -p a/arch/m68k/math-emu/fp_log.c b/arch/m68k/math-emu/fp_log.c
> > --- a/arch/m68k/math-emu/fp_log.c 2011-06-13 14:06:37.943954469 -0500
> > +++ b/arch/m68k/math-emu/fp_log.c 2011-06-14 16:07:22.394954040 -0500
> > @@ -105,9 +105,6 @@ fp_fetoxm1(struct fp_ext *dest, struct f
> >
> > fp_monadic_check(dest, src);
> >
> > - if (IS_ZERO(dest))
> > - return dest;
> > -
> > return dest;
> > }
> >
> >
> >
> > Here's an example where it got it "wrong" - notice how the blank line is
> > missing the - :
> >
> > diff -u -p a/arch/frv/mm/pgalloc.c b/arch/frv/mm/pgalloc.c
> > --- a/arch/frv/mm/pgalloc.c 2011-06-13 14:06:37.855954391 -0500
> > +++ b/arch/frv/mm/pgalloc.c 2011-06-14 16:07:16.714954008 -0500
> > @@ -136,8 +136,6 @@ pgd_t *pgd_alloc(struct mm_struct *mm)
> > pgd_t *pgd;
> >
> > pgd = quicklist_alloc(0, GFP_KERNEL, pgd_ctor);
> > - if (!pgd)
> > - return pgd;
> >
> > return pgd;
> > }

OK, but it is going to be hard for Coccinelle to know that, although the 
programmer previously thought that return should be separated from the 
rest of the function by a blank line, that is now no longer the case.  
Perhaps it is due to the fact that there is now only one other line in the 
body of the function, but it seems like an opinion as to how to draw the 
line.

So your - return ret; + return ret; is probably the appropriate solution.  
You want to get rid of the whole pattern if (...) return ret; return ret; 
and replace it with just return ret;, which will then be inserted at the 
point of the beginning of the match to the pattern.

It would be nicer to put the - return ret; + return ret; inside the last 
line of the ( | ).  Then only those return ret's are rewritten rather than 
every return ret in the program.  It should improver performance and 
reduce the risk of changing spacing.  The other ifs in the ( | ) don't 
need to be followed by return ret.  They are just ifs that you want to 
ignore completely.

julia

Re: [Cocci] Re: [PATCH v2] coccinelle: if (ret) return ret; return ret; semantic patch

[Greg Dietsche]

On 06/15/2011 12:58 AM, Julia Lawall wrote:
> On Tue, 14 Jun 2011, Greg Dietsche wrote:
>
>>
>>
>> On 06/14/2011 04:24 PM, Greg Dietsche wrote:
>>> On 06/14/2011 12:50 AM, Julia Lawall wrote:
>>>> On Mon, 13 Jun 2011, Greg Dietsche wrote:
>>>>> just curious... i see you usually just write "return ret;" here when
>>>>> posting.
>>>>> I've assumed that's because it will 1) work and 2) is close enough.
>>>>>
>>>>> You'll notice I've been doing:
>>>>> -return ret;
>>>>> +return ret;
>>>>> because it seems to help coccinelle realize that it can get rid of
>>>>> extra line
>>>>> feeds - does this make sense - or should i just be doing a "return ret"?
>>>> I wondered why you were doing that :)
>>>>
>>>> Is the problem that the removed if is being replaced by a blank line? If
>>>> so, that is not intentional. I will look into it. If it doesn't happen
>>>> always, an example where it does happen could be helpful.
>>>>
>>>
>>> Some times it gets it right, so it's not always wrong. For example:
>>>
>>> diff -u -p a/arch/m68k/math-emu/fp_log.c b/arch/m68k/math-emu/fp_log.c
>>> --- a/arch/m68k/math-emu/fp_log.c 2011-06-13 14:06:37.943954469 -0500
>>> +++ b/arch/m68k/math-emu/fp_log.c 2011-06-14 16:07:22.394954040 -0500
>>> @@ -105,9 +105,6 @@ fp_fetoxm1(struct fp_ext *dest, struct f
>>>
>>> fp_monadic_check(dest, src);
>>>
>>> - if (IS_ZERO(dest))
>>> - return dest;
>>> -
>>> return dest;
>>> }
>>>
>>>
>>>
>>> Here's an example where it got it "wrong" - notice how the blank line is
>>> missing the - :
>>>
>>> diff -u -p a/arch/frv/mm/pgalloc.c b/arch/frv/mm/pgalloc.c
>>> --- a/arch/frv/mm/pgalloc.c 2011-06-13 14:06:37.855954391 -0500
>>> +++ b/arch/frv/mm/pgalloc.c 2011-06-14 16:07:16.714954008 -0500
>>> @@ -136,8 +136,6 @@ pgd_t *pgd_alloc(struct mm_struct *mm)
>>> pgd_t *pgd;
>>>
>>> pgd = quicklist_alloc(0, GFP_KERNEL, pgd_ctor);
>>> - if (!pgd)
>>> - return pgd;
>>>
>>> return pgd;
>>> }
>
> OK, but it is going to be hard for Coccinelle to know that, although the
> programmer previously thought that return should be separated from the
> rest of the function by a blank line, that is now no longer the case.
> Perhaps it is due to the fact that there is now only one other line in the
> body of the function, but it seems like an opinion as to how to draw the
> line.
>

OK - I can see how that would be hard for Coccinelle to guess what we 
really want in this case.

> So your - return ret; + return ret; is probably the appropriate solution.
> You want to get rid of the whole pattern if (...) return ret; return ret;
> and replace it with just return ret;, which will then be inserted at the
> point of the beginning of the match to the pattern.

ok

> It would be nicer to put the - return ret; + return ret; inside the last
> line of the ( | ).  Then only those return ret's are rewritten rather than
> every return ret in the program.  It should improver performance and

except that 4 of the 5 ORs are cases where we want to do the -return 
ret; + return ret; So I suppose for performance, I should actually add 
the +/- to each of the 4 cases that we want cocci to generate a patch for?


thanks,
Greg

> reduce the risk of changing spacing.  The other ifs in the ( | ) don't
> need to be followed by return ret.  They are just ifs that you want to
> ignore completely.
>
> julia

Re: [Cocci] Re: [PATCH v2] coccinelle: if (ret) return ret; return ret; semantic patch

[Julia Lawall]

On Wed, 15 Jun 2011, Greg Dietsche wrote:

> On 06/15/2011 12:58 AM, Julia Lawall wrote:
> > On Tue, 14 Jun 2011, Greg Dietsche wrote:
> >
> > >
> > >
> > > On 06/14/2011 04:24 PM, Greg Dietsche wrote:
> > > > On 06/14/2011 12:50 AM, Julia Lawall wrote:
> > > > > On Mon, 13 Jun 2011, Greg Dietsche wrote:
> > > > > > just curious... i see you usually just write "return ret;" here when
> > > > > > posting.
> > > > > > I've assumed that's because it will 1) work and 2) is close enough.
> > > > > >
> > > > > > You'll notice I've been doing:
> > > > > > -return ret;
> > > > > > +return ret;
> > > > > > because it seems to help coccinelle realize that it can get rid of
> > > > > > extra line
> > > > > > feeds - does this make sense - or should i just be doing a "return
> > > > > > ret"?
> > > > > I wondered why you were doing that :)
> > > > >
> > > > > Is the problem that the removed if is being replaced by a blank line?
> > > > > If
> > > > > so, that is not intentional. I will look into it. If it doesn't happen
> > > > > always, an example where it does happen could be helpful.
> > > > >
> > > >
> > > > Some times it gets it right, so it's not always wrong. For example:
> > > >
> > > > diff -u -p a/arch/m68k/math-emu/fp_log.c b/arch/m68k/math-emu/fp_log.c
> > > > --- a/arch/m68k/math-emu/fp_log.c 2011-06-13 14:06:37.943954469 -0500
> > > > +++ b/arch/m68k/math-emu/fp_log.c 2011-06-14 16:07:22.394954040 -0500
> >>> @@ -105,9 +105,6 @@ fp_fetoxm1(struct fp_ext *dest, struct f
> > > >
> > > > fp_monadic_check(dest, src);
> > > >
> > > > - if (IS_ZERO(dest))
> > > > - return dest;
> > > > -
> > > > return dest;
> >>> }
> > > >
> > > >
> > > >
> > > > Here's an example where it got it "wrong" - notice how the blank line is
> > > > missing the - :
> > > >
> > > > diff -u -p a/arch/frv/mm/pgalloc.c b/arch/frv/mm/pgalloc.c
> > > > --- a/arch/frv/mm/pgalloc.c 2011-06-13 14:06:37.855954391 -0500
> > > > +++ b/arch/frv/mm/pgalloc.c 2011-06-14 16:07:16.714954008 -0500
> >>> @@ -136,8 +136,6 @@ pgd_t *pgd_alloc(struct mm_struct *mm)
> > > > pgd_t *pgd;
> > > >
> > > > pgd = quicklist_alloc(0, GFP_KERNEL, pgd_ctor);
> > > > - if (!pgd)
> > > > - return pgd;
> > > >
> > > > return pgd;
> >>> }
> >
> > OK, but it is going to be hard for Coccinelle to know that, although the
> > programmer previously thought that return should be separated from the
> > rest of the function by a blank line, that is now no longer the case.
> > Perhaps it is due to the fact that there is now only one other line in the
> > body of the function, but it seems like an opinion as to how to draw the
> > line.
> >
> 
> OK - I can see how that would be hard for Coccinelle to guess what we really
> want in this case.
> 
> > So your - return ret; + return ret; is probably the appropriate solution.
> > You want to get rid of the whole pattern if (...) return ret; return ret;
> > and replace it with just return ret;, which will then be inserted at the
> > point of the beginning of the match to the pattern.
> 
> ok
> 
> > It would be nicer to put the - return ret; + return ret; inside the last
> > line of the ( | ).  Then only those return ret's are rewritten rather than
> > every return ret in the program.  It should improver performance and
> 
> except that 4 of the 5 ORs are cases where we want to do the -return ret; +
> return ret; So I suppose for performance, I should actually add the +/- to
> each of the 4 cases that we want cocci to generate a patch for?

Yes.  Or you could at least see if it helps.

julia

Re: [Cocci] Re: [PATCH v2] coccinelle: if (ret) return ret; return ret; semantic patch

[Julia Lawall]

> except that 4 of the 5 ORs are cases where we want to do the -return ret; +
> return ret; So I suppose for performance, I should actually add the +/- to
> each of the 4 cases that we want cocci to generate a patch for?

Actually, it's probably not that big a deal.  It's only rewriting the ones 
where there is a return ret just before, not all the returns in the code, 
like I was originally thinking.

julia

[PATCH v4] coccinelle: if (ret) return ret; return ret; semantic patch

[Greg Dietsche]

Semantic patch to find code that matches this pattern:
	if (...) return ret;
	return ret;

Version 2:
	Incorporate review comments from Julia Lawall
	Add matches for a number of other similar patterns.

Version 3:
	Incorporate more review comments from Julia.

Version 4:
	Add -return ret; +return ret; back to the script
	as this improves Coccinelle's handling of white
	space for this patch.

Signed-off-by: Greg Dietsche <Gregory.Dietsche@cuw.edu>
---
 scripts/coccinelle/misc/doublereturn.cocci |   32 ++++++++++++++++++++++++++++
 1 files changed, 32 insertions(+), 0 deletions(-)
 create mode 100644 scripts/coccinelle/misc/doublereturn.cocci

diff --git a/scripts/coccinelle/misc/doublereturn.cocci b/scripts/coccinelle/misc/doublereturn.cocci
new file mode 100644
index 0000000..3fe40f8
--- /dev/null
+++ b/scripts/coccinelle/misc/doublereturn.cocci
@@ -0,0 +1,32 @@
+// Removing unecessary code that matches this core pattern:
+//	-if(...) return ret;
+//	return ret;
+//
+// Confidence: High
+// Copyright: (C) 2011 Greg Dietsche GPLv2.
+// URL: http://www.gregd.org
+// Comments:
+// Options: -no_includes
+
+virtual patch
+
+@depends on patch@
+expression ret;
+identifier x, y;
+identifier f;
+identifier is_ordinal_table ~= "IS_ORDINAL_TABLE_\(ONE\|TWO\)";
+@@
+(
+//via an isomorphism this also covers x and unlikely(x)
+-if (likely(x)) return ret;
+|
+-if (\(IS_ERR\|IS_ZERO\)(x)) return ret;
+|
+-if (is_ordinal_table(x,y)) return ret;
+|
+if(<+...f(...)...+>) return ret;
+|
+-if (...) return ret;
+)
+-return ret;
++return ret;
-- 
1.7.2.5

Re: [Cocci] Re: [PATCH v2] coccinelle: if (ret) return ret; return ret; semantic patch

[Greg Dietsche]

On Wed, Jun 15, 2011 at 05:35:47PM +0200, Julia Lawall wrote:
> > except that 4 of the 5 ORs are cases where we want to do the -return ret; +
> > return ret; So I suppose for performance, I should actually add the +/- to
> > each of the 4 cases that we want cocci to generate a patch for?
> 
> Actually, it's probably not that big a deal.  It's only rewriting the ones 
> where there is a return ret just before, not all the returns in the code, 
> like I was originally thinking.

Ok :) I'll update the patch one last time - hopefully we've got everything nailed
down now :)

thanks for being so incredibly helpful!

Greg

> 
> julia
>