From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751474AbcGWVFr (ORCPT ); Sat, 23 Jul 2016 17:05:47 -0400 Received: from mx1.mailbox.org ([80.241.60.212]:51940 "EHLO mx1.mailbox.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751301AbcGWVFq (ORCPT ); Sat, 23 Jul 2016 17:05:46 -0400 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 To: Nadim almas References: <1469182637-4419-1-git-send-email-nadim.902@gmail.com> Cc: gregkh@linuxfoundation.org, devel@driverdev.osuosl.org, mmarek@suse.com, linux-kernel@vger.kernel.org, julia.lawall@lip6.fr From: =?UTF-8?Q?Markus_B=c3=b6hme?= Message-ID: <13fa9fa8-0bdd-f001-19dc-fd44a1efffa4@mailbox.org> Date: Sat, 23 Jul 2016 23:02:02 +0200 MIME-Version: 1.0 In-Reply-To: <1469182637-4419-1-git-send-email-nadim.902@gmail.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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. > --- > 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