From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751449AbcGWVKt (ORCPT ); Sat, 23 Jul 2016 17:10:49 -0400 Received: from mail3-relais-sop.national.inria.fr ([192.134.164.104]:42612 "EHLO mail3-relais-sop.national.inria.fr" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751301AbcGWVKs (ORCPT ); Sat, 23 Jul 2016 17:10:48 -0400 X-IronPort-AV: E=Sophos;i="5.28,410,1464645600"; d="scan'208";a="185738703" Date: Sat, 23 Jul 2016 23:10:42 +0200 (CEST) From: Julia Lawall X-X-Sender: jll@hadrien To: =?ISO-8859-15?Q?Markus_B=F6hme?= cc: Nadim almas , gregkh@linuxfoundation.org, devel@driverdev.osuosl.org, mmarek@suse.com, linux-kernel@vger.kernel.org Subject: Re: [PATCH 32/32] Staging/octeon-usb/octeon-hcd.c: Compression of lines for immediate return This patch compresses two lines in to a single line in file octeon-hcd.c In-Reply-To: <13fa9fa8-0bdd-f001-19dc-fd44a1efffa4@mailbox.org> Message-ID: References: <1469182637-4419-1-git-send-email-nadim.902@gmail.com> <13fa9fa8-0bdd-f001-19dc-fd44a1efffa4@mailbox.org> User-Agent: Alpine 2.10 (DEB 1266 2009-07-14) MIME-Version: 1.0 Content-Type: MULTIPART/MIXED; BOUNDARY="8323329-1003753187-1469308245=:3027" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org This message is in MIME format. The first part should be readable text, while the remaining parts are likely unreadable without MIME-aware tools. --8323329-1003753187-1469308245=:3027 Content-Type: TEXT/PLAIN; charset=windows-1252 Content-Transfer-Encoding: 8BIT On Sat, 23 Jul 2016, Markus Böhme wrote: > Nadim, > > several points stand out in your patch: > > On 07/22/2016 12:17 PM, Nadim almas wrote: > > if immediate return statement is found. It also removes variable > > bytes_written as > > it is no longer needed. > > > > It is done using script Coccinelle. And coccinelle uses following > > semantic > > patch for this compression function: > > > > @@ > > expression ret; > > identifier f; > > @@ > > > > -ret = > > +return > > f(...); > > -return ret; > > The commit message is malformed. Start a new paragraph for the longer > description of what you are doing after the short one. > > > > > Signed-off-by: Nadim Almas > > Acked-by: Julia Lawall > > If Julia acked this patch she probably should be copied on this mail. No, I haven't seen it... And I don't think the above semantic patch should have made these changes. julia > > > --- > > Makefile | 2 +- > > drivers/staging/octeon-usb/octeon-hcd.c | 16 ++++++++-------- > > 2 files changed, 9 insertions(+), 9 deletions(-) > > > > diff --git a/Makefile b/Makefile > > index 4fb6bea..3d9d77a6 100644 > > --- a/Makefile > > +++ b/Makefile > > @@ -1,7 +1,7 @@ > > VERSION = 4 > > PATCHLEVEL = 7 > > SUBLEVEL = 0 > > -EXTRAVERSION = -rc4 > > +EXTRAVERSION = -eudyptula-rc4 > > No need to change this. > > > NAME = Psychotic Stoned Sheep > > > > # *DOCUMENTATION* > > diff --git a/drivers/staging/octeon-usb/octeon-hcd.c b/drivers/staging/octeon-usb/octeon-hcd.c > > index 17442b3..b801c8a 100644 > > --- a/drivers/staging/octeon-usb/octeon-hcd.c > > +++ b/drivers/staging/octeon-usb/octeon-hcd.c > > @@ -508,15 +508,15 @@ static int octeon_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb, > > { > > int ret; > > > > - ret = octeon_alloc_temp_buffer(urb, mem_flags); > > - if (ret) > > - return ret; > > + > > + if (octeon_alloc_temp_buffer(urb, mem_flags)) > > + return octeon_alloc_temp_buffer(urb, mem_flags); > > This cannot possibly be correct! You are calling a function with side > effects twice. The first call in the condition might fail while the call > in the return statement might succeed, never minding the wastefulness of > two identical calls. Besides, the original code here seems fine to me. > > > > > - ret = usb_hcd_map_urb_for_dma(hcd, urb, mem_flags); > > - if (ret) > > + > > + if (usb_hcd_map_urb_for_dma(hcd, urb, mem_flags)) > > octeon_free_temp_buffer(urb); > > > > - return ret; > > + return usb_hcd_map_urb_for_dma(hcd, urb, mem_flags); > > } > > Same here. > > > > > /** > > @@ -542,8 +542,8 @@ static void octeon_unmap_urb_for_dma(struct usb_hcd *hcd, struct urb *urb) > > */ > > static inline u32 cvmx_usb_read_csr32(struct octeon_hcd *usb, u64 address) > > { > > - u32 result = cvmx_read64_uint32(address ^ 4); > > - return result; > > + > > + return cvmx_read64_uint32(address ^ 4); > > } > > > > /** > > > > This change looks fine, though. > > Automatically generating patches does not free you from validating each > of them manually. > > Regards, > Markus > --8323329-1003753187-1469308245=:3027--