[PATCH v2] [Outreachy kernel] staging: rtl8188eu: hal: remove an unneeded variable

[PATCH v2] [Outreachy kernel] staging: rtl8188eu: hal: remove an unneeded variable

[Payal Kshirsagar]

Remove unneeded temporary local variable, cleanup suggested by coccinelle.

Signed-off-by: Payal Kshirsagar <payalskshirsagar1234@gmail.com>
---
v2: Change in commit message and changelog.

 drivers/staging/rtl8188eu/hal/usb_halinit.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/staging/rtl8188eu/hal/usb_halinit.c b/drivers/staging/rtl8188eu/hal/usb_halinit.c
index 16a57b31a439..aa6354b9ffa2 100644
--- a/drivers/staging/rtl8188eu/hal/usb_halinit.c
+++ b/drivers/staging/rtl8188eu/hal/usb_halinit.c
@@ -40,8 +40,6 @@ static void _ConfigNormalChipOutEP_8188E(struct adapter *adapt, u8 NumOutPipe)
 
 static bool HalUsbSetQueuePipeMapping8188EUsb(struct adapter *adapt, u8 NumInPipe, u8 NumOutPipe)
 {
-	bool			result		= false;
-
 	_ConfigNormalChipOutEP_8188E(adapt, NumOutPipe);
 
 	/*  Normal chip with one IN and one OUT doesn't have interrupt IN EP. */
@@ -52,9 +50,7 @@ static bool HalUsbSetQueuePipeMapping8188EUsb(struct adapter *adapt, u8 NumInPip
 
 	/*  All config other than above support one Bulk IN and one Interrupt IN. */
 
-	result = hal_mapping_out_pipe(adapt, NumOutPipe);
-
-	return result;
+	return hal_mapping_out_pipe(adapt, NumOutPipe);
 }
 
 void rtw_hal_chip_configure(struct adapter *adapt)
-- 
2.17.1

Re: [PATCH v2] [Outreachy kernel] staging: rtl8188eu: hal: remove an unneeded variable

[Stefano Brivio]

On Wed, 11 Mar 2020 13:04:18 +0530
Payal Kshirsagar <payalskshirsagar1234@gmail.com> wrote:

> Remove unneeded temporary local variable, cleanup suggested by coccinelle.
> 
> Signed-off-by: Payal Kshirsagar <payalskshirsagar1234@gmail.com>
> ---
> v2: Change in commit message and changelog.

Note, mostly for the future: especially if changes to a patch are not
trivial, it's a good practice to attribute suggestions to their
authors. If the notes come before "---" (as it's the case for
networking changes) that also ends up in the git history.

For example, this could have been:

v2: Change in commit message and changelog (as suggested by Julia
    Lawall)

or even:

v2: Change in commit message and changelog (Julia)

Two examples (there might be more complicated ones or different styles,
I just could remember and quickly find these):

commit 1e47b4837f3b ("ipv6: Dump route exceptions if requested")
commit 3c4287f62044 ("nf_tables: Add set type for arbitrary concatenation of ranges")

Anyway, it looks good to me!

Reviewed-by: Stefano Brivio <sbrivio@redhat.com>

-- 
Stefano

Re: [PATCH v2] [Outreachy kernel] staging: rtl8188eu: hal: remove an unneeded variable

[Payal Kshirsagar]

[-- Attachment #1: Type: text/plain, Size: 1267 bytes --]

On Wed, Mar 11, 2020, 5:28 PM Stefano Brivio <sbrivio@redhat.com> wrote:

> On Wed, 11 Mar 2020 13:04:18 +0530
> Payal Kshirsagar <payalskshirsagar1234@gmail.com> wrote:
>
> > Remove unneeded temporary local variable, cleanup suggested by
> coccinelle.
> >
> > Signed-off-by: Payal Kshirsagar <payalskshirsagar1234@gmail.com>
> > ---
> > v2: Change in commit message and changelog.
>
> Note, mostly for the future: especially if changes to a patch are not
> trivial, it's a good practice to attribute suggestions to their
> authors. If the notes come before "---" (as it's the case for
> networking changes) that also ends up in the git history.
>
> For example, this could have been:
>
> v2: Change in commit message and changelog (as suggested by Julia
>     Lawall)
>
> or even:
>
> v2: Change in commit message and changelog (Julia)
>

Okay, I will take care from next time.

Two examples (there might be more complicated ones or different styles,
> I just could remember and quickly find these):
>
> commit 1e47b4837f3b ("ipv6: Dump route exceptions if requested")
> commit 3c4287f62044 ("nf_tables: Add set type for arbitrary concatenation
> of ranges")
>
> Anyway, it looks good to me!
>
> Reviewed-by: Stefano Brivio <sbrivio@redhat.com>
>
> --
> Stefano
>
>

[-- Attachment #2: Type: text/html, Size: 2192 bytes --]

Re: [PATCH v2] [Outreachy kernel] staging: rtl8188eu: hal: remove an unneeded variable

[Greg KH]

On Wed, Mar 11, 2020 at 01:04:18PM +0530, Payal Kshirsagar wrote:
> Remove unneeded temporary local variable, cleanup suggested by coccinelle.
> 
> Signed-off-by: Payal Kshirsagar <payalskshirsagar1234@gmail.com>
> ---
> v2: Change in commit message and changelog.
> 
>  drivers/staging/rtl8188eu/hal/usb_halinit.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/drivers/staging/rtl8188eu/hal/usb_halinit.c b/drivers/staging/rtl8188eu/hal/usb_halinit.c
> index 16a57b31a439..aa6354b9ffa2 100644
> --- a/drivers/staging/rtl8188eu/hal/usb_halinit.c
> +++ b/drivers/staging/rtl8188eu/hal/usb_halinit.c
> @@ -40,8 +40,6 @@ static void _ConfigNormalChipOutEP_8188E(struct adapter *adapt, u8 NumOutPipe)
>  
>  static bool HalUsbSetQueuePipeMapping8188EUsb(struct adapter *adapt, u8 NumInPipe, u8 NumOutPipe)
>  {
> -	bool			result		= false;
> -
>  	_ConfigNormalChipOutEP_8188E(adapt, NumOutPipe);
>  
>  	/*  Normal chip with one IN and one OUT doesn't have interrupt IN EP. */
> @@ -52,9 +50,7 @@ static bool HalUsbSetQueuePipeMapping8188EUsb(struct adapter *adapt, u8 NumInPip
>  
>  	/*  All config other than above support one Bulk IN and one Interrupt IN. */
>  
> -	result = hal_mapping_out_pipe(adapt, NumOutPipe);
> -
> -	return result;
> +	return hal_mapping_out_pipe(adapt, NumOutPipe);
>  }
>  
>  void rtw_hal_chip_configure(struct adapter *adapt)
> -- 
> 2.17.1
> 

Always test-build your patches so that you do not get grumpy kernel
maintainers telling you to test-build your patches to prevent the build
from breaking.

Are you _SURE_ you used coccinelle for this?  If so, something is really
broken as this patch is totally incorrect :(

The suggestion might be correct, but the implementation is broken.

thanks,

greg k-h

Re: [PATCH v2] [Outreachy kernel] staging: rtl8188eu: hal: remove an unneeded variable

[Payal Kshirsagar]

On Thu, 2020-03-12 at 09:27 +0100, Greg KH wrote:
> On Wed, Mar 11, 2020 at 01:04:18PM +0530, Payal Kshirsagar wrote:
> > Remove unneeded temporary local variable, cleanup suggested by
> > coccinelle.
> > 
> > Signed-off-by: Payal Kshirsagar <payalskshirsagar1234@gmail.com>
> > ---
> > v2: Change in commit message and changelog.
> > 
> >  drivers/staging/rtl8188eu/hal/usb_halinit.c | 6 +-----
> >  1 file changed, 1 insertion(+), 5 deletions(-)
> > 
> > diff --git a/drivers/staging/rtl8188eu/hal/usb_halinit.c
> > b/drivers/staging/rtl8188eu/hal/usb_halinit.c
> > index 16a57b31a439..aa6354b9ffa2 100644
> > --- a/drivers/staging/rtl8188eu/hal/usb_halinit.c
> > +++ b/drivers/staging/rtl8188eu/hal/usb_halinit.c
> > @@ -40,8 +40,6 @@ static void _ConfigNormalChipOutEP_8188E(struct
> > adapter *adapt, u8 NumOutPipe)
> >  
> >  static bool HalUsbSetQueuePipeMapping8188EUsb(struct adapter
> > *adapt, u8 NumInPipe, u8 NumOutPipe)
> >  {
> > -	bool			result		= false;
> > -
> >  	_ConfigNormalChipOutEP_8188E(adapt, NumOutPipe);
> >  
> >  	/*  Normal chip with one IN and one OUT doesn't have interrupt
> > IN EP. */
> > @@ -52,9 +50,7 @@ static bool
> > HalUsbSetQueuePipeMapping8188EUsb(struct adapter *adapt, u8
> > NumInPip
> >  
> >  	/*  All config other than above support one Bulk IN and one
> > Interrupt IN. */
> >  
> > -	result = hal_mapping_out_pipe(adapt, NumOutPipe);
> > -
> > -	return result;
> > +	return hal_mapping_out_pipe(adapt, NumOutPipe);
> >  }
> >  
> >  void rtw_hal_chip_configure(struct adapter *adapt)
> > -- 
> > 2.17.1
> > 
> 
> Always test-build your patches so that you do not get grumpy kernel
> maintainers telling you to test-build your patches to prevent the
> build
> from breaking.
> 

HOW TO test-build the patches?


> Are you _SURE_ you used coccinelle for this?  If so, something is
> really
> broken as this patch is totally incorrect :(
> 
> The suggestion might be correct, but the implementation is broken.
> 
> thanks,
> 
> greg k-h

Yes, I used following semantic patch to get this file for cleanup
@@
local idexpression ret;
expression e;
@@

-ret =
+return
     e;
-return ret;

Re: [PATCH v2] [Outreachy kernel] staging: rtl8188eu: hal: remove an unneeded variable

[Julia Lawall]

> > Always test-build your patches so that you do not get grumpy kernel
> > maintainers telling you to test-build your patches to prevent the
> > build
> > from breaking.
> >
>
> HOW TO test-build the patches?

Compile the code and be sure you obtain a .o file for the file you
changed.  If you don't you may need to adjust the configuration variables.


>
>
> > Are you _SURE_ you used coccinelle for this?  If so, something is
> > really
> > broken as this patch is totally incorrect :(
> >
> > The suggestion might be correct, but the implementation is broken.
> >
> > thanks,
> >
> > greg k-h
>
> Yes, I used following semantic patch to get this file for cleanup
> @@
> local idexpression ret;
> expression e;
> @@
>
> -ret =
> +return
>      e;
> -return ret;

The semantic patch is fine, but you also deleted the variable declaration.
If you use a tool, it is best to make clear how you used it and what you
did beyond what the tool said to do.

julia


> --
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/13eb3ca2d7db751231b3a755e1609e385ce3c36a.camel%40gmail.com.
>

Re: [PATCH v2] [Outreachy kernel] staging: rtl8188eu: hal: remove an unneeded variable

[Payal Kshirsagar]

On Thu, 2020-03-12 at 13:37 +0100, Julia Lawall wrote:
> > > Always test-build your patches so that you do not get grumpy
> > > kernel
> > > maintainers telling you to test-build your patches to prevent the
> > > build
> > > from breaking.
> > > 
> > 
> > HOW TO test-build the patches?
> 
> Compile the code and be sure you obtain a .o file for the file you
> changed.  If you don't you may need to adjust the configuration
> variables.
> 
> 
> > 
> > 
> > > Are you _SURE_ you used coccinelle for this?  If so, something is
> > > really
> > > broken as this patch is totally incorrect :(
> > > 
> > > The suggestion might be correct, but the implementation is
> > > broken.
> > > 
> > > thanks,
> > > 
> > > greg k-h
> > 
> > Yes, I used following semantic patch to get this file for cleanup
> > @@
> > local idexpression ret;
> > expression e;
> > @@
> > 
> > -ret =
> > +return
> >      e;
> > -return ret;
> 
> The semantic patch is fine, but you also deleted the variable
> declaration.
> If you use a tool, it is best to make clear how you used it and what
> you
> did beyond what the tool said to do.
> 

I deleted the declaration because it will be of no use if I delete
other two lines which is the only use of that variable.
I am Extremly sorry, but I am not understanding what need to be done.
 

> julia
> 
> 
> > --
> > You received this message because you are subscribed to the Google
> > Groups "outreachy-kernel" group.
> > To unsubscribe from this group and stop receiving emails from it,
> > send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> > To view this discussion on the web visit 
> > https://groups.google.com/d/msgid/outreachy-kernel/13eb3ca2d7db751231b3a755e1609e385ce3c36a.camel%40gmail.com
> > .
> > 

Re: [PATCH v2] [Outreachy kernel] staging: rtl8188eu: hal: remove an unneeded variable

[Julia Lawall]

> I deleted the declaration because it will be of no use if I delete
> other two lines which is the only use of that variable.
> I am Extremly sorry, but I am not understanding what need to be done.

As far as I could see there was another use of the variable in an if
branch.  Those can be easy to overlook.  Just be sure that you get a .o
file after compiling the code that you changed and you should be mostly
ok.

julia

Re: [PATCH v2] [Outreachy kernel] staging: rtl8188eu: hal: remove an unneeded variable

[Payal Kshirsagar]

On March 12, 2020 6:29:51 PM GMT+05:30, Julia Lawall <julia.lawall@inria.fr> wrote:
>
>> I deleted the declaration because it will be of no use if I delete
>> other two lines which is the only use of that variable.
>> I am Extremly sorry, but I am not understanding what need to be done.
>
>As far as I could see there was another use of the variable in an if
>branch.  Those can be easy to overlook.  Just be sure that you get a .o
>file after compiling the code that you changed and you should be mostly
>ok.
>

Okay, Thank You:)

>julia