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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
>
>
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 <[email protected]>
> ---
> 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
>
>
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
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
Hi,
On Tue, Jun 7, 2011 at 6:56 PM, Julia Lawall <[email protected]> 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/
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
Hi,
On Wed, Jun 8, 2011 at 12:59 AM, Greg Dietsche <[email protected]> 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/
On Wed, 8 Jun 2011, Nicolas Palix wrote:
> Hi,
>
> On Wed, Jun 8, 2011 at 12:59 AM, Greg Dietsche <[email protected]> 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
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<[email protected]>
>> ---
>> 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
>>
>>
>>
>
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<[email protected]>
> > > ---
> > > 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
> > >
> > >
> > >
> >
>
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 <[email protected]>
---
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
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 >=
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 <[email protected]>
> ---
> 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
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
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
>
>
>
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
>
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;
}
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
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
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
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
> 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
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 <[email protected]>
---
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
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
>