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

From: Phillip Potter <phil@philpotter.co.uk>
To: luo.penghao@zte.com.cn
Cc: cgel.zte@gmail.com, linux-kernel@vger.kernel.org,
	zealci@zte.com.cn, linux-block@vger.kernel.org
Subject: Re: [PATCH linux-next] cdrom: Remove redundant variable and its assignment.
Date: Tue, 19 Oct 2021 23:58:38 +0100	[thread overview]
Message-ID: <YW9NnuSZ5aWZrcm2@equinox> (raw)
In-Reply-To: <202110191004079619787@zte.com.cn>

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