Re: [net-next PATCH 04/10] bitfield.h: add FIELD_WIDTH()

[Luiz Angelo Daros de Luca]

> > +/**
> > + * FIELD_WIDTH() - return the width of a bitfield
> > + * @_mask: shifted mask defining the field's length and position
> > + *
> > + * Returns the number of contiguous bits covered by @_mask.
> > + * This corresponds to the bit width of FIELD_MAX(@_mask).
> > + */
> > +#define FIELD_WIDTH(_mask)                                           \
>
> Please no underscored names unless necessary.

I used _mask to maintain consistency with the existing public macros
in this file, such as FIELD_GET, FIELD_PREP, FIELD_MAX, and FIELD_FIT.
All of them use the underscore prefix for parameters. Should I diverge
from them?

> > +     ({                                                              \
> > +             __BF_FIELD_CHECK(_mask, 0ULL, 0ULL, "FIELD_WIDTH: ");   \
> > +             __bf_shf(~FIELD_MAX(_mask));                            \
> > +     })
>
> I believe, this should be:
>
>   #define FIELD_WIDTH(mask) ({                                  \
>         __BF_FIELD_CHECK_MASK(mask, 0ULL, "FIELD_WIDTH: ");     \
>         HWEIGHT(mask);                                          \
>   })

HWEIGHT() is indeed much cleaner. However, to keep bitfield.h
self-contained and avoid adding more includes, I'll try
__builtin_popcountll() in a similar way __builtin_ffsll is already
used.

I also noticed the suggestion to use __BF_FIELD_CHECK_MASK instead of
__BF_FIELD_CHECK. Both FIELD_MAX and FIELD_FIT currently use the full
__BF_FIELD_CHECK with 0ULL as a dummy register to ensure the mask fits
within the header's supported limits. If __BF_FIELD_CHECK_MASK is
preferred for FIELD_WIDTH, I can certainly use it, but should we also
update FIELD_MAX and FIELD_FIT for consistency?

I intend to send a v2 with the following implementation:

#define __bf_shf(x) (__builtin_ffsll(x) - 1)
+#define __bf_hweight(x) __builtin_popcountll(x)

#define __scalar_type_to_unsigned_cases(type)                          \
               unsigned type:  (unsigned type)0,                       \
@@ -111,6 +112,19 @@
               (typeof(_mask))((_mask) >> __bf_shf(_mask));            \
       })

+/**
+ * FIELD_WIDTH() - return the width of a bitfield
+ * @_mask: shifted mask defining the field's length and position
+ *
+ * Returns the number of contiguous bits covered by @_mask.
+ * This corresponds to the bit width of FIELD_MAX(@_mask).
+ */
+#define FIELD_WIDTH(_mask)                                             \
+       ({                                                              \
+               __BF_FIELD_CHECK_MASK(_mask, 0ULL, "FIELD_WIDTH: ");   \
+               (typeof(_mask))__bf_hweight(_mask);                     \
+       })
+

Regards,

Luiz