lkml.org 
[lkml]   [2011]   [Jun]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v2] coccinelle: if (ret) return ret; return ret; semantic patch
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


\
 
 \ /
  Last update: 2011-06-15 08:01    [W:0.090 / U:27.880 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site