Re: [PATCH 4/9] bitfield: Copy #define parameters to locals
From: Andy Shevchenko <hidden>
Date: 2025-12-09 21:54:41
Also in:
linux-usb, lkml
On Tue, Dec 09, 2025 at 07:11:48PM +0000, David Laight wrote:
On Tue, 9 Dec 2025 17:51:48 +0200 Andy Shevchenko [off-list ref] wrote:quoted
On Tue, Dec 09, 2025 at 10:03:08AM +0000, david.laight.linux@gmail.com wrote:
...
quoted
quoted
-#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