Re: [dpdk-dev] [PATCH v10 01/10] eal: introduce macro for bit definition

[Thomas Monjalon <thomas@monjalon.net>]

28/07/2020 17:39, Honnappa Nagarahalli:
> > On 28/07/20 10:24 +0200, Morten Brørup wrote:
> > > > From: Honnappa Nagarahalli [mailto:Honnappa.Nagarahalli@arm.com]
> > > > > > > --- a/lib/librte_eal/include/rte_bitops.h
> > > > > > > +++ b/lib/librte_eal/include/rte_bitops.h
> > > > > > > @@ -17,6 +17,14 @@
> > > > > > >  #include <rte_debug.h>
> > > > > > >  #include <rte_compat.h>
> > > > > > >
> > > > > > > +/**
> > > > > > > + * Get the uint64_t value for a specified bit set.
> > > > > > > + *
> > > > > > > + * @param nr
> > > > > > > + *   The bit number in range of 0 to 63.
> > > > > > > + */
> > > > > > > +#define RTE_BIT64(nr) (UINT64_C(1) << (nr))
> > > > > > In general, the macros have been avoided in this file. Suggest
> > > > > > changing this to an inline function.
> > > > >
> > > > > That has been discussed already, and rejected for good reasons:
> > > > >
> > http://inbox.dpdk.org/dev/AM0PR05MB4866823B0170B90F679A2765D1640@
> > > > > AM0PR05MB4866.eurprd05.prod.outlook.com/
> > > > Thank you for the link.
> > > > In this patch series, I see the macro being used in enum
> > > > initialization
> > > > (7/10 in v11) as well as in functions (8/10 in v11). Does it make
> > > > sense to introduce use inline functions and use the inline functions
> > > > for 8/10?
> > > > If we do this, we should document in rte_bitops.h that inline
> > > > functions should be used wherever possible.
> > >
> > > I would agree, but only in theory. I disagree in reality, and argue that there
> > should only be macros for this. Here is why:
> > >
> > > rte_byteorder.h has both RTE_BEnn() macros and rte_cpu_to_be_nn()
> > functions, for doing the same thing at compile time or at run time. There are
> > no compile time warnings if the wrong one is being used, so I am certain that
> > we can find code that uses the macro where the function should be used, or
> > vice versa.
> Agree, there is not a suitable way to enforce the use of one over the other (other than code review).
> 
> When the APIs in rte_bitops.h were introduced, there was a discussion around using the macros. I was for using macros as it would have kept the code as well as number of APIs smaller. However, there was a decision made not to use macros and instead provide inline functions. It was nothing to do with performance. So, I am just saying that we need to follow the same principles at least for this file.

I think bit definitions should be simple macros.
Even macro is a bit overkill for this simple thing.
I will merge this series.

If we want to change the philosophy of macro definitions,
it may be a larger discussion.