28/07/2020 17:39, Honnappa Nagarahalli: > > On 28/07/20 10:24 +0200, Morten Brørup wrote: > > > > From: Honnappa Nagarahalli [mailto:honnappa.nagaraha...@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.
