On 22.11.2023 02:43, Stefano Stabellini wrote:
> On Mon, 20 Nov 2023, Julien Grall wrote:
>> On 18/11/2023 02:46, Stefano Stabellini wrote:
>>> On Fri, 17 Nov 2023, Andrew Cooper wrote:
>>>> On 17/11/2023 10:17 am, Nicola Vetrini wrote:
>>>>> Hi all,
>>>>>
>>>>> As discussed in this thread [1], which is about complying with MISRA C
>>>>> Rule 10.1,
>>>>> a macro was introduced to encapsulate a well-known construct:
>>>>>
>>>>> /*
>>>>> * Given an unsigned integer argument, expands to a mask where just
>>>>> the least
>>>>> * significant nonzero bit of the argument is set, or 0 if no bits are
>>>>> set.
>>>>> */
>>>>> #define ISOLATE_LSB(x) ((x) & -(x))
>>>>>
>>>>> This macro has a gained some calls in the subsequent patches in that
>>>>> thread, but concerns were raised around the fact that it would be
>>>>> better to devise a macro that evaluates its argument only once. A
>>>>> proposed solution is this (thanks to Jan Beulich):
>>>>>
>>>>> #define ISOLATE_LSB(x) ({ \
>>>>> typeof(x) x_ = (x); \
>>>>> x_ & -x_; \
>>>>> })
>>>>
>>>> Of course this was going to explode.
>>>>
>>>> This isn't even the first time an unwise attempt to do single-evaluation
>>>> has needed to be reverted because it doesn't work with Integer Constant
>>>> Expressions.
>>>>
>>>> Switch it back to the first form. It's obviously a macro to begin with,
>>>> and not likely to be used in cases that have side effects.
>>>
>>> +1
>>
>> FWIW +1. I don't much like the idea to have two different versions of the
>> helper if there is no real need for it.
>
> Jan, would you be willing to accept that other maintainers have a
> preference for having a single MACRO even if suboptimal?
I can live with that, even if I'm surprised by this perspective that others
take. How can we, in reviews, tell people to make sure arguments are
evaluated only once, when then we deliberately do otherwise in a case like
the one here? The criteria of "not likely to be used in cases that have
side effects" is an extremely fuzzy and sufficiently weak one, imo. I for
one am even worried about the uses in MASK_EXTR() / MASK_INSR(), and would
have considered introducing single-evaluation forms there as well.
> If so, can we go ahead and commit the original patches?
Well, the renaming needs to be done there anyway.
Jan
