Re: [Qemu-devel] [PATCH] ipmi: Add parenthesis around some macro parameters

[Corey Minyard]

On 12/22/2016 09:01 AM, Eric Blake wrote:
> On 12/22/2016 08:30 AM, address@hidden wrote:
> > From: Corey Minyard <address@hidden>
> >
> > Macro parameters should almost always have () around them when used.
> > llvm reported an error on this.
> >
> > Reported in https://bugs.launchpad.net/bugs/1651167
> >
> > Signed-off-by: Corey Minyard <address@hidden>
> > ---
> >   hw/ipmi/isa_ipmi_bt.c | 18 +++++++++---------
> >   1 file changed, 9 insertions(+), 9 deletions(-)
> >
> > diff --git a/hw/ipmi/isa_ipmi_bt.c b/hw/ipmi/isa_ipmi_bt.c
> > index f036617..8a97314 100644
> > --- a/hw/ipmi/isa_ipmi_bt.c
> > +++ b/hw/ipmi/isa_ipmi_bt.c
> > @@ -40,37 +40,37 @@
> >   #define IPMI_BT_CLR_WR_MASK        (1 << IPMI_BT_CLR_WR_BIT)
> >   #define IPMI_BT_GET_CLR_WR(d)      (((d) >> IPMI_BT_CLR_WR_BIT) & 0x1)
> >   #define IPMI_BT_SET_CLR_WR(d, v)   (d) = (((d) & ~IPMI_BT_CLR_WR_MASK) | \
> Still under-parenthesized, if the result of IPMI_BT_SET_CLR_WR() is used
> in any larger expression;

I wasn't thinking about this being used in a larger expression, but it 
should be protected,
I suppose.  I'll re-submit with that fixed and the extra () removed.

Thanks,

-corey

> > -                                       (((v & 1) << IPMI_BT_CLR_WR_BIT)))
> > +                                       ((((v) & 1) << IPMI_BT_CLR_WR_BIT)))
> and at the same time, you (still) have a redundant set on the second line.
>
> Better would be:
>
> ((d) = (((d) & ~IPMI_BD_CLR_WR_MASK) | \
>          (((v) & 1) << IPMI_BT_CLR_WR_BIT)))
>
> >   
> >   #define IPMI_BT_CLR_RD_MASK        (1 << IPMI_BT_CLR_RD_BIT)
> >   #define IPMI_BT_GET_CLR_RD(d)      (((d) >> IPMI_BT_CLR_RD_BIT) & 0x1)
> >   #define IPMI_BT_SET_CLR_RD(d, v)   (d) = (((d) & ~IPMI_BT_CLR_RD_MASK) | \
> > -                                       (((v & 1) << IPMI_BT_CLR_RD_BIT)))
> > +                                       ((((v) & 1) << IPMI_BT_CLR_RD_BIT)))
> and again, throughout the patch.