| Message ID | 1482417047-15588-1-git-send-email-minyard@acm.org |
|---|---|
| State | New |
| Headers | show |
On 12/22/2016 08:30 AM, minyard@acm.org wrote: > From: Corey Minyard <cminyard@mvista.com> > > 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 <cminyard@mvista.com> > --- > 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; > - (((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.
On 12/22/2016 09:01 AM, Eric Blake wrote: > On 12/22/2016 08:30 AM, minyard@acm.org wrote: >> From: Corey Minyard <cminyard@mvista.com> >> >> 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 <cminyard@mvista.com> >> --- >> 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. > >
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) | \ - (((v & 1) << IPMI_BT_CLR_WR_BIT))) + ((((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))) #define IPMI_BT_H2B_ATN_MASK (1 << IPMI_BT_H2B_ATN_BIT) #define IPMI_BT_GET_H2B_ATN(d) (((d) >> IPMI_BT_H2B_ATN_BIT) & 0x1) #define IPMI_BT_SET_H2B_ATN(d, v) (d) = (((d) & ~IPMI_BT_H2B_ATN_MASK) | \ - (((v & 1) << IPMI_BT_H2B_ATN_BIT))) + ((((v) & 1) << IPMI_BT_H2B_ATN_BIT))) #define IPMI_BT_B2H_ATN_MASK (1 << IPMI_BT_B2H_ATN_BIT) #define IPMI_BT_GET_B2H_ATN(d) (((d) >> IPMI_BT_B2H_ATN_BIT) & 0x1) #define IPMI_BT_SET_B2H_ATN(d, v) (d) = (((d) & ~IPMI_BT_B2H_ATN_MASK) | \ - (((v & 1) << IPMI_BT_B2H_ATN_BIT))) + ((((v) & 1) << IPMI_BT_B2H_ATN_BIT))) #define IPMI_BT_SMS_ATN_MASK (1 << IPMI_BT_SMS_ATN_BIT) #define IPMI_BT_GET_SMS_ATN(d) (((d) >> IPMI_BT_SMS_ATN_BIT) & 0x1) #define IPMI_BT_SET_SMS_ATN(d, v) (d) = (((d) & ~IPMI_BT_SMS_ATN_MASK) | \ - (((v & 1) << IPMI_BT_SMS_ATN_BIT))) + ((((v) & 1) << IPMI_BT_SMS_ATN_BIT))) #define IPMI_BT_HBUSY_MASK (1 << IPMI_BT_HBUSY_BIT) #define IPMI_BT_GET_HBUSY(d) (((d) >> IPMI_BT_HBUSY_BIT) & 0x1) #define IPMI_BT_SET_HBUSY(d, v) (d) = (((d) & ~IPMI_BT_HBUSY_MASK) | \ - (((v & 1) << IPMI_BT_HBUSY_BIT))) + ((((v) & 1) << IPMI_BT_HBUSY_BIT))) #define IPMI_BT_BBUSY_MASK (1 << IPMI_BT_BBUSY_BIT) #define IPMI_BT_GET_BBUSY(d) (((d) >> IPMI_BT_BBUSY_BIT) & 0x1) #define IPMI_BT_SET_BBUSY(d, v) (d) = (((d) & ~IPMI_BT_BBUSY_MASK) | \ - (((v & 1) << IPMI_BT_BBUSY_BIT))) + ((((v) & 1) << IPMI_BT_BBUSY_BIT))) /* Mask register */ @@ -80,12 +80,12 @@ #define IPMI_BT_B2H_IRQ_EN_MASK (1 << IPMI_BT_B2H_IRQ_EN_BIT) #define IPMI_BT_GET_B2H_IRQ_EN(d) (((d) >> IPMI_BT_B2H_IRQ_EN_BIT) & 0x1) #define IPMI_BT_SET_B2H_IRQ_EN(d, v) (d) = (((d) & ~IPMI_BT_B2H_IRQ_EN_MASK) | \ - (((v & 1) << IPMI_BT_B2H_IRQ_EN_BIT))) + ((((v) & 1) << IPMI_BT_B2H_IRQ_EN_BIT))) #define IPMI_BT_B2H_IRQ_MASK (1 << IPMI_BT_B2H_IRQ_BIT) #define IPMI_BT_GET_B2H_IRQ(d) (((d) >> IPMI_BT_B2H_IRQ_BIT) & 0x1) #define IPMI_BT_SET_B2H_IRQ(d, v) (d) = (((d) & ~IPMI_BT_B2H_IRQ_MASK) | \ - (((v & 1) << IPMI_BT_B2H_IRQ_BIT))) + ((((v) & 1) << IPMI_BT_B2H_IRQ_BIT))) typedef struct IPMIBT { IPMIBmc *bmc;