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

[Andy Shevchenko <andriy.shevchenko@intel.com>]

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