[PATCH v2 05/12] staging: ks7010: remove unnecessary parenthesis

[Tobin C. Harding]

On Thu, Mar 09, 2017 at 09:30:40PM +0100, Greg Kroah-Hartman wrote:
> On Fri, Mar 10, 2017 at 07:08:28AM +1100, Tobin C. Harding wrote:
> > On Thu, Mar 09, 2017 at 02:04:20PM +0100, Greg Kroah-Hartman wrote:
> > > On Thu, Mar 09, 2017 at 03:47:05PM +1100, Tobin C. Harding wrote:
> > > > Checkpatch emits CHECK: Unnecessary parentheses.
> > > > 
> > > > Remove unnecessary parentheses.
> > > > 
> > > > Signed-off-by: Tobin C. Harding <me at tobin.cc>
> > > > ---
> > > >  drivers/staging/ks7010/ks_hostif.c | 24 ++++++++++++------------
> > > >  1 file changed, 12 insertions(+), 12 deletions(-)
> > > > 
> > > > diff --git a/drivers/staging/ks7010/ks_hostif.c b/drivers/staging/ks7010/ks_hostif.c
> > > > index a354e34e..b75ef1d6 100644
> > > > --- a/drivers/staging/ks7010/ks_hostif.c
> > > > +++ b/drivers/staging/ks7010/ks_hostif.c
> > > > @@ -36,7 +36,7 @@ inline u8 get_BYTE(struct ks_wlan_private *priv)
> > > >  {
> > > >  	u8 data;
> > > >  
> > > > -	data = *(priv->rxp)++;
> > > > +	data = *priv->rxp++;
> > > 
> > > Are you sure this is ok?  I would have to dig out a book to find the
> > > ordering rules here...
> > 
> > I also had to get out K&R. The reason I decided to look it up and make
> > the change wast that with the parenthesis I also still needed to think
> > about the precedence. Adding the parenthesis in no way makes the
> > precedence *more* clear. And the checkpatch warning of course.
> > 
> > I ran this code to check it
> > 
> > #include <stdio.h>
> > #include <stdint.h>
> > 
> > /*
> >  * Confirm precedence 
> >  */
> > 
> > int main(void)
> > {
> >         uint8_t buf[3];
> >         uint8_t *bp = buf;
> >         uint8_t data;
> > 
> >         buf[0] = 0;
> >         buf[1] = 1;
> >         buf[2] = 2;
> > 
> >         printf("buf = [%d, %d, %d]\n",(int)buf[0], (int)buf[1], (int)buf[2]);
> >         printf("bp before: %d\n", (int)*bp);
> >   
> >         /* data = *(bp)++; */
> >         data = *bp++;
> > 
> >         printf("bp after: %d\n", (int)*bp);
> > 
> >         return(0);
> > }
> 
> Great, so everyone has to check it is valid, so please leave it alone.
> We write code for people to read, and be able to modify in the future.
> Don't force someone to have to really think hard about something trivial
> like this.

Understood. Will drop this change from next version.

Greg please stop reading this email now.

thanks,
Tobin.


For correctness; the dummy code above is not complete. It should be

#include <stdio.h>
#include <stdint.h>

/*
 * Confirm statements equivalent
 *
 * data = *(priv->rxp)++;
 * data = *priv->rxp++;
 */

int one = 1;
int two = 2;

struct foo_t {
	int *v;
}foos[] = {
	{&one},
	{&two},
};

int main(void)
{
	struct foo_t *fp = foos;

	int data = -1;

	printf("before: fp->v: %d (%d) data: %d\n", *fp->v, *(fp->v), data);

	data = *(fp->v)++;
	/* data = *fp->v++; */

	printf("after: fp->v: %d (%d) data: %d\n", *fp->v, *(fp->v), data);

	return(0);
}

I was wrong *(foo->baz)++ is easier to read than *foo->baz++

thanks,
Tobin.