> From: Morten Brørup <m...@smartsharesystems.com> > Sent: Thursday, July 9, 2020 12:46 PM > > > From: dev [mailto:dev-boun...@dpdk.org] On Behalf Of Parav Pandit > > Sent: Thursday, July 9, 2020 8:24 AM > > > > Hi Morten, > > > > > From: Morten Brørup <m...@smartsharesystems.com> > > > Sent: Tuesday, July 7, 2020 6:11 PM > > > > > 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. > > Ok. will add. > > > > > > > > > > > > > > > > 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); \ > > > }) > > > > > Compiler doesn't like it. > > > > ../lib/librte_eal/include/rte_bitops.h:21:2: error: braced-group > > within expression allowed only inside a function > > ({ \ > > ^ > > > > > 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; > > > } > > > > > Value retrieved using this macro is used an enum. Don't see how a > > function call like above can solve it. > > > > For a below macro definition, compiler is already catching for > > negative value when RTE_BIT64(-1) is done, > > > > ../lib/librte_eal/include/rte_bitops.h:36:36: warning: left shift > > count is negative [-Wshift-count-negative] #define RTE_BIT64(nr) > > (UINT64_C(1) << (nr)) > > > > And when RTE_BIT64(259) is done below error is done, > > > > ../lib/librte_eal/include/rte_bitops.h:36:36: warning: left shift > > count > > >= width of type [-Wshift-count-overflow] > > #define RTE_BIT64(nr) (UINT64_C(1) << (nr)) > > > > So below definition is good covering all needed cases. > > > > #define RTE_BIT64(nr) (UINT64_C(1) << (nr)) > > Great. Then, when you have added a doxygen comment: > > Acked-by: Morten Brørup <m...@smartsharesystems.com>
Thanks Morten; adding it.
