Re: [PATCH linux-next] cdrom: Remove redundant variable and its assignment.

Re: [PATCH linux-next] cdrom: Remove redundant variable and its assignment.

[Phillip Potter]

On Mon, Oct 18, 2021 at 09:08:34AM +0000, luo penghao wrote:
> From: penghao luo <luo.penghao@zte.com.cn>
> 
> Variable is not used in functions, and its assignment is redundant too.
> So it should be deleted.
> 
> The clang_analyzer complains as follows:
> 
> drivers/cdrom/cdrom.c:877: warning:
> 
> Although the value stored to 'ret' is used in the enclosing expression,
> the value is never actually read from 'ret’.
> 
> Reported-by: Zeal Robot <zealci@zte.com.cn>
> Signed-off-by: penghao luo <luo.penghao@zte.com.cn>
> ---

Dear Penghao,

Thank you for the patch, looks good, but please could I ask for a small
tweak:

>  drivers/cdrom/cdrom.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c
> index 89a6845..393acf4 100644
> --- a/drivers/cdrom/cdrom.c
> +++ b/drivers/cdrom/cdrom.c
> @@ -871,7 +871,7 @@ static void cdrom_mmc3_profile(struct cdrom_device_info *cdi)
>  {
>  	struct packet_command cgc;
>  	char buffer[32];
> -	int ret, mmc3_profile;
> +	int mmc3_profile;
>  
>  	init_cdrom_command(&cgc, buffer, sizeof(buffer), CGC_DATA_READ);
>  
> @@ -881,7 +881,7 @@ static void cdrom_mmc3_profile(struct cdrom_device_info *cdi)
>  	cgc.cmd[8] = sizeof(buffer);		/* Allocation Length */
>  	cgc.quiet = 1;
>  
> -	if ((ret = cdi->ops->generic_packet(cdi, &cgc)))
> +	if ((cdi->ops->generic_packet(cdi, &cgc)))

We no longer need the inner-most set of parentheses now, as we are
checking the result of the expression:
cdi->ops->generic_packet(cdi, &cgc)

rather than the result of the assignment expression:
(ret = cdi->ops->generic_packet(cdi, &cgc))

Please resubmit with this change and I'd be happy to approve the patch.
Many thanks.

Regards,
Phil

Re: [PATCH linux-next] cdrom: Remove redundant variable and its assignment.

[Phillip Potter]

On Tue, Oct 19, 2021 at 10:04:07AM +0800, luo.penghao@zte.com.cn wrote:
> > We no longer need the inner-most set of parentheses now, as we are> checking the result of the expression:> cdi->ops->generic_packet(cdi, &cgc)> rather than the result of the assignment expression:> (ret = cdi->ops->generic_packet(cdi, &cgc))> Please resubmit with this change and I'd be happy to approve the patch.> Many thanks.>Regards,> Phil
> 
> 
> Thans for your response. Actually I have found several such writings, 
> 
> 
> when I looked at the kernel code.such as
> 
> 
> > (drivers/video/fbdev/sis/sis_main.c  2498)
> 
> >       if((result = SISDoSense(ivideo, svhs, 0x0604))) {
> 
> >           if((result = SISDoSense(ivideo, cvbs, 0x0804))) {
> 
> > 	     printk(KERN_INFO "%s %s YPbPr component output\n", stdstr, tvstr);
> 
> > 	     SiS_SetRegOR(SISCR, 0x32, 0x80);
> 
> > 	  }
> 
> >        }
> 
> 
> I thought the doubel parentheses was a special expression，which I cannot understand(just for me).
> 
> 
> So I didn't modify it easily.

Dear Penghao,

So the reason assignment expressions are wrapped like this when used as
if conditions is that compilers will often by default issues warnings
otherwise - the compiler will warn to check that you didn't mean:
if (x == y)

rather than:
if (x = y)

which is a common mistake. Using the extra parentheses lets the compiler
know we really did mean to do an assignment, not an equality check.

Semantically however, there is no difference between:
if (x = y)

and:
if ((x = y))

in terms of the ultimate evaluation of the expression.

Another reason for wrapping in my opinion is good practice, as later on
one may wish to add additional operators to the condition. For example,
if we wanted to check if the result was less than another value:
if ((x = y) < z)

has a very different meaning to:
if (x = y < z)

due to the assignment operator having a much lower precedence than other
operators. The extra parentheses therefore enforce precedence here, and
add clarity as well. Hope this helps.

Since your change removes the assignment entirely, the extra
parentheses are therefore not required. Hope this helps. As mentioned,
by all means resubmit with this tweak and I will happily accept the
patch.

Regards,
Phil

Re: [PATCH linux-next] cdrom: Remove redundant variable and its assignment.

[Phillip Potter]

On Wed, Oct 20, 2021 at 02:42:29AM +0000, luo penghao wrote:
> Variable is not used in functions, and its assignment is redundant too.
> So it should be deleted. Also the inner-most set of parentheses is no
> longer needed.
> 
> The clang_analyzer complains as follows:
> 
> drivers/cdrom/cdrom.c:877: warning:
> 
> Although the value stored to 'ret' is used in the enclosing expression,
> the value is never actually read from 'ret'.
> 
> Reported-by: Zeal Robot <zealci@zte.com.cn>
> Signed-off-by: luo penghao <luo.penghao@zte.com.cn>
> ---
>  drivers/cdrom/cdrom.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c
> index feb827e..40970b8 100644
> --- a/drivers/cdrom/cdrom.c
> +++ b/drivers/cdrom/cdrom.c
> @@ -864,7 +864,7 @@ static void cdrom_mmc3_profile(struct cdrom_device_info *cdi)
>  {
>  	struct packet_command cgc;
>  	char buffer[32];
> -	int ret, mmc3_profile;
> +	int mmc3_profile;
>  
>  	init_cdrom_command(&cgc, buffer, sizeof(buffer), CGC_DATA_READ);
>  
> @@ -874,7 +874,7 @@ static void cdrom_mmc3_profile(struct cdrom_device_info *cdi)
>  	cgc.cmd[8] = sizeof(buffer);		/* Allocation Length */
>  	cgc.quiet = 1;
>  
> -	if ((ret = cdi->ops->generic_packet(cdi, &cgc)))
> +	if (cdi->ops->generic_packet(cdi, &cgc))
>  		mmc3_profile = 0xffff;
>  	else
>  		mmc3_profile = (buffer[6] << 8) | buffer[7];
> -- 
> 2.15.2
> 
> 

Looks good. I will send through to Jens Axboe in the morning (UK time),
as we are at rc6 now so merge window will be open shortly I'd imagine.
Many thanks for your contribution.

Regards,
Phil