Comment 3 for bug 1651167

On 12/22/2016 09:01 AM, Eric Blake wrote:
> On 12/22/2016 08:30 AM, <email address hidden> wrote:
>> From: Corey Minyard <email 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 <email 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.
>
>