| Message ID | 1490970783-29231-1-git-send-email-minyard@acm.org |
|---|---|
| State | New |
| Headers | show |
On 03/31/2017 09:33 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. > > Remove redundant parenthesis and put parenthesis around the entire > macros with assignments in case they are used in an expression. > > The macros were doing ((v) & 1) for a binary input, but that only works > if v == 0 or if v & 1. Changed to !!(v) so they work for all values. s/&/==/ > > Remove some unused macros. > > Reported in https://bugs.launchpad.net/bugs/1651167 Might also be worth adding that an audit of the code finds no semantic change, that this is just cleaning up the compiler warning. > > Signed-off-by: Corey Minyard <cminyard@mvista.com> > --- > > Changes since the last revision: > > * Added the !!(v) change > > hw/ipmi/isa_ipmi_bt.c | 34 ++++++++++++---------------------- > 1 file changed, 12 insertions(+), 22 deletions(-) Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 03/31/2017 10:04 AM, Eric Blake wrote: > On 03/31/2017 09:33 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. >> >> Remove redundant parenthesis and put parenthesis around the entire >> macros with assignments in case they are used in an expression. >> >> The macros were doing ((v) & 1) for a binary input, but that only works >> if v == 0 or if v & 1. Changed to !!(v) so they work for all values. > s/&/==/ > I originally wrote v == 1, but I realized that it worked with anything where the bottom bit of v was set. Thus, v & 1. >> Remove some unused macros. >> >> Reported in https://bugs.launchpad.net/bugs/1651167 > Might also be worth adding that an audit of the code finds no semantic > change, that this is just cleaning up the compiler warning. Will do. Thanks, -corey > >> Signed-off-by: Corey Minyard <cminyard@mvista.com> >> --- >> >> Changes since the last revision: >> >> * Added the !!(v) change >> >> hw/ipmi/isa_ipmi_bt.c | 34 ++++++++++++---------------------- >> 1 file changed, 12 insertions(+), 22 deletions(-) > Reviewed-by: Eric Blake <eblake@redhat.com> >
On 31 March 2017 at 11:04, Eric Blake <eblake@redhat.com> wrote: > > Might also be worth adding that an audit of the code finds no semantic > change, that this is just cleaning up the compiler warning. We should include core detail of your detailed analysis in such a statement I think, because I it's more than just cleaning up a warning. For me at least the warning pointed out that the expression is not parsed would be expected (if this were a function and not a macro), and it's just because of constraints on the inputs that there's no functional change. >> Signed-off-by: Corey Minyard <cminyard@mvista.com> > Reviewed-by: Eric Blake <eblake@redhat.com> Reviewed-by: Ed Maste <emaste@freebsd.org>
diff --git a/hw/ipmi/isa_ipmi_bt.c b/hw/ipmi/isa_ipmi_bt.c index 1c69cb3..13a8c09 100644 --- a/hw/ipmi/isa_ipmi_bt.c +++ b/hw/ipmi/isa_ipmi_bt.c @@ -37,40 +37,30 @@ #define IPMI_BT_HBUSY_BIT 6 #define IPMI_BT_BBUSY_BIT 7 -#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))) -#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))) -#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))) #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))) +#define IPMI_BT_SET_B2H_ATN(d, v) ((d) = (((d) & ~IPMI_BT_B2H_ATN_MASK) | \ + (!!(v) << 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))) +#define IPMI_BT_SET_SMS_ATN(d, v) ((d) = (((d) & ~IPMI_BT_SMS_ATN_MASK) | \ + (!!(v) << 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))) +#define IPMI_BT_SET_HBUSY(d, v) ((d) = (((d) & ~IPMI_BT_HBUSY_MASK) | \ + (!!(v) << 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))) +#define IPMI_BT_SET_BBUSY(d, v) ((d) = (((d) & ~IPMI_BT_BBUSY_MASK) | \ + (!!(v) << IPMI_BT_BBUSY_BIT))) /* Mask register */ @@ -79,13 +69,13 @@ #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))) +#define IPMI_BT_SET_B2H_IRQ_EN(d, v) ((d) = (((d) & ~IPMI_BT_B2H_IRQ_EN_MASK) |\ + (!!(v) << 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))) +#define IPMI_BT_SET_B2H_IRQ(d, v) ((d) = (((d) & ~IPMI_BT_B2H_IRQ_MASK) | \ + (!!(v) << IPMI_BT_B2H_IRQ_BIT))) typedef struct IPMIBT { IPMIBmc *bmc;