On 03/02/2025 at 16:44, Johannes Berg wrote:
> On Sun, 2025-02-02 at 12:53 -0500, Yury Norov wrote:
>>
>>> Instead of creating another variant for
>>> non-constant bitfields, wouldn't it be better to make the existing macro
>>> accept both?
>>
>> Yes, it would definitely be better IMO.
>
> On the flip side, there have been discussions in the past (though I
> think not all, if any, on the list(s)) about the argument order. Since
> the value is typically not a constant, requiring the mask to be a
> constant has ensured that the argument order isn't as easily mixed up as
> otherwise.
If this is a concern, then it can be checked with:
BUILD_BUG_ON_MSG(!__builtin_constant_p(_mask) &&
__builtin_constant_p(_val),
_pfx "mask is not constant");
It means that we forbid FIELD_PREP(non_const_mask, const_val) but allow
any other combination.
> With a non-constant mask there can also be no validation that the mask
> is contiguous etc.
>
> Now that doesn't imply a strong objection - personally I've come to
> prefer the lower-case typed versions anyway - but something to keep in
> mind when doing this.
>
> However, the suggested change to BUILD_BUG_ON_NOT_POWER_OF_2 almost
> certainly shouldn't be done for the same reason - not compiling for non-
> constant values is [IMHO] part of the API contract for that macro. This
> can be important for the same reasons.
Your point is fair enough. But I do not see this as a killer argument.
We can instead just add below helper:
BUILD_BUG_ON_STATICALLY_NOT_POWER_OF_2()
But, for the same reason why I would rather not have both the
FIELD_{PREP,GET}() and the field_{prep,get}(), I would also rather not
have a BUILD_BUG_ON_NOT_POWER_OF_2() and a
BUILD_BUG_ON_STATICALLY_NOT_POWER_OF_2().
If your concern is the wording of the contract, the description can just
be updated.
Yours sincerely,
Vincent Mailhol
