Adding Joyce Kong to this discussion as the rte_bitops maintainer.
> From: Thomas Monjalon [mailto:tho...@monjalon.net]
> Sent: Tuesday, July 7, 2020 2:13 PM
>
> 07/07/2020 13:38, Parav Pandit:
> > From: Morten Brørup <m...@smartsharesystems.com>
> > > From: Parav Pandit
> > > > --- a/lib/librte_eal/include/rte_bitops.h
> > > > +++ b/lib/librte_eal/include/rte_bitops.h
> > > > @@ -17,6 +17,8 @@
> > > > #include <rte_debug.h>
> > > > #include <rte_compat.h>
> > > >
> > > > +#define RTE_BIT(bit_num) (1UL << (bit_num))
> > >
> > > Is the return value 32 or 64 bit, or is intended to depend on the
> target
> > > architecture?
> > >
> > It should be 64-bit.
> >
> > > Please be explicit by using UINT32_C(1) or UINT64_C(1) instead of
> 1UL, if you
> > > want a specific size.
> > >
> > Will do UINT64_C(1).
> >
> > > It could be a static inline __attribute__((__pure__)) function
> instead of a macro,
> > > but it's not important for me.
> > >
> > > The macro/function needs a description for the documentation.
> > >
> > In this header file or outside?
>
> It is asked to add a doxygen comment.
>
>
> > > I'm also concerned about the name of the macro being too generic.
> But the
> > > effort of changing all the drivers where it is being used already
> could be too big
> > > if the name changes too.
> > >
> > Right. Currently drivers have generic name as BIT(). Close to 3000
> entries.
> > So doing at RTE_BIT to match other rte_ APIs.
> > Drivers can slowly migrate at their pace to this one.
> >
> > > And the macro/function is new, so shouldn't it - in theory - be
> marked as
> > > experimental?
> >
> > How to mark a macro as experimental?
>
> A macro cannot be experimental.
>
OK. If the macro is given a future proof name, I guess it should be accepted.
If we want boundary checks, I suggest a macro like:
#define RTE_BIT64(nr) \
({ \
typeof(nr) n = nr; \
RTE_BUILD_BUG_ON((n > 64) || (n < 0)); \
UINT64_C(1) << (n); \
})
Or a function:
__rte_experimental
static __rte_always_inline __attribute__((const)) uint64_t
rte_bit64(const unsigned int nr)
{
RTE_ASSERT(nr < 64);
return UINT64_C(1) << nr;
}
