[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

[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

[Nadim almas]

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;

Signed-off-by: Nadim Almas<nadim.902@gmail.com>
Acked-by: Julia Lawall <julia.lawall@lip6.fr>
---
 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
 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);
 
-	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);
 }
 
 /**
@@ -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);
 }
 
 /**
-- 
2.7.4

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

[Markus Böhme]

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@gmail.com>
> Acked-by: Julia Lawall <julia.lawall@lip6.fr>

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

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]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 3106 bytes --]



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@gmail.com>
> > Acked-by: Julia Lawall <julia.lawall@lip6.fr>
>
> 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
>