Re: Devise macros to encapsulate (x & -x)

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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?

If so, can we go ahead and commit the original patches?