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

[Julia Lawall]

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<nadim.902@xxxxxxxxx>
> > Acked-by: Julia Lawall <julia.lawall@xxxxxxx>
>
> 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
>