Message-ID: <Pine.LNX.4.64.1106150751390.27735@ask.diku.dk>
Date:	Wed, 15 Jun 2011 07:58:38 +0200 (CEST)
From:	Julia Lawall <julia@...u.dk>
To:	Greg Dietsche <greg@...mergreg.com>
Cc:	Gilles.Muller@...6.fr, npalix.work@...il.com, cocci@...u.dk,
	linux-kernel@...r.kernel.org
Subject: Re: [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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists