Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752226Ab1FOF6p (ORCPT ); Wed, 15 Jun 2011 01:58:45 -0400 Received: from mgw2.diku.dk ([130.225.96.92]:35809 "EHLO mgw2.diku.dk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751410Ab1FOF6o (ORCPT ); Wed, 15 Jun 2011 01:58:44 -0400 Date: Wed, 15 Jun 2011 07:58:38 +0200 (CEST) From: Julia Lawall To: Greg Dietsche Cc: Gilles.Muller@lip6.fr, npalix.work@gmail.com, cocci@diku.dk, linux-kernel@vger.kernel.org Subject: Re: [PATCH v2] coccinelle: if (ret) return ret; return ret; semantic patch In-Reply-To: <4DF807B4.8010807@gregd.org> Message-ID: References: <1307989386-17666-1-git-send-email-Gregory.Dietsche@cuw.edu> <4DF67933.9080707@cuw.edu> <4DF7D197.5070205@cuw.edu> <4DF807B4.8010807@gregd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3180 Lines: 82 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/