On 05/03/2025 at 23:33, Andy Shevchenko wrote: > On Wed, Mar 05, 2025 at 10:00:16PM +0900, Vincent Mailhol via B4 Relay wrote: >> From: Lucas De Marchi <lucas.demarchi@xxxxxxxxx> >> >> Implement fixed-type BIT to help drivers add stricter checks, like was > > Here and in the Subject I would use BIT_Uxx(). > >> done for GENMASK(). > > ... > >> +/* >> + * Fixed-type variants of BIT(), with additional checks like GENMASK_t(). The > > GENMASK_t() is not a well named macro. Ack. I will rename to GENMASK_TYPE(). >> + * following examples generate compiler warnings due to shift-count-overflow: >> + * >> + * - BIT_U8(8) >> + * - BIT_U32(-1) >> + * - BIT_U32(40) >> + */ >> +#define BIT_INPUT_CHECK(type, b) \ >> + BUILD_BUG_ON_ZERO(const_true((b) >= BITS_PER_TYPE(type))) >> + >> +#define BIT_U8(b) (BIT_INPUT_CHECK(u8, b) + (unsigned int)BIT(b)) >> +#define BIT_U16(b) (BIT_INPUT_CHECK(u16, b) + (unsigned int)BIT(b)) > > Why not u8 and u16? This inconsistency needs to be well justified. Because of the C integer promotion rules, if casted to u8 or u16, the expression will immediately become a signed integer as soon as it is get used. For example, if casted to u8 BIT_U8(0) + BIT_U8(1) would be a signed integer. And that may surprise people. David also pointed this in the v3: https://lore.kernel.org/intel-xe/d42dc197a15649e69d459362849a37f2@xxxxxxxxxxxxxxxx/ and I agree with his comment. I explained this in the changelog below the --- cutter, but it is probably better to make the explanation more visible. I will add a comment in the code to explain this. >> +#define BIT_U32(b) (BIT_INPUT_CHECK(u32, b) + (u32)BIT(b)) >> +#define BIT_U64(b) (BIT_INPUT_CHECK(u64, b) + (u64)BIT_ULL(b)) > > Can you also use a TAB between the parentheses for better readability? > E.g., > > #define BIT_U64(b)r (BIT_INPUT_CHECK(u64, b) + (u64)BIT_ULL(b)) Sure. I prefer it with space, but no strong opinion. I will put tab in v5. Yours sincerely, Vincent Mailhol
