Re: [PATCH 4/9] bitfield: Copy #define parameters to locals

Andy Shevchenko
2025-12-09T21:54:41Z

On Tue, Dec 09, 2025 at 07:11:48PM +0000, David Laight wrote:
> On Tue, 9 Dec 2025 17:51:48 +0200
> Andy Shevchenko <andriy.shevchenko@intel.com> wrote:
> > On Tue, Dec 09, 2025 at 10:03:08AM +0000, david.laight.linux@gmail.com wrote:

...

> > > -#define __BF_FIELD_CHECK_MASK(_mask, _val, _pfx) \
> > > +#define __BF_FIELD_CHECK_MASK(mask, val, pfx) \
> > > ({ \
> > > - BUILD_BUG_ON_MSG(!__builtin_constant_p(_mask), \
> > > - _pfx "mask is not constant"); \
> > > - BUILD_BUG_ON_MSG((_mask) == 0, _pfx "mask is zero"); \
> > > - BUILD_BUG_ON_MSG(__builtin_constant_p(_val) ? \
> > > - ~((_mask) >> __bf_shf(_mask)) & \
> > > - (0 + (_val)) : 0, \
> > > - _pfx "value too large for the field"); \
> > > - __BUILD_BUG_ON_NOT_POWER_OF_2((_mask) + \
> > > - (1ULL << __bf_shf(_mask))); \
> > > + BUILD_BUG_ON_MSG(!__builtin_constant_p(mask), \
> > > + pfx "mask is not constant"); \
> > > + BUILD_BUG_ON_MSG((mask) == 0, _pfx "mask is zero"); \
> > > + BUILD_BUG_ON_MSG(__builtin_constant_p(val) ? \
> > > + ~((mask) >> __bf_shf(mask)) & \
> > > + (0 + (val)) : 0, \
> > > + pfx "value too large for the field"); \
> > > + __BUILD_BUG_ON_NOT_POWER_OF_2((mask) + \
> > > + (1ULL << __bf_shf(mask))); \
> > > })
> >
> > I looks like renaming parameters without any benefit, actually the opposite
> > it's very hard to see if there is any interesting change here. Please, drop
> > this or make it clear to focus only on the things that needs to be changed.
>
> I'm pretty sure there are no other changes in that bit.

Yes, but the rule of thumb to avoid putting several logical changes into a
single patch and here AFAICT the renaming should be avoided / split to a
precursor or do it after this.

> (The entire define is pretty much re-written in a later patch and I
> did want to separate the changes.)

Then probably don't do the change at all (renaming), as it's useless here?

> I wanted to the file to be absolutely consistent with the parameter/variable
> names.

No objection on this.

> Plausibly the scheme could be slightly different:
> 'user' parameters are 'xxx', '__auto_type' variables are '_xxx'.
> But internal defines that evaluate/expand parameters more than once are
> '_xxx' and must be 'copied' by an outer define.

--
With Best Regards,
Andy Shevchenko