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;
}


Reply via email to