Re: [dpdk-dev] [PATCH] librte_eal/common: fix return type of rte_bsf64

["Morten Brørup" <mb@smartsharesystems.com>]

CC: ABI Policy maintainers. You might have an opinion. Or not. :-)

> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Tyler Retzlaff
> Sent: Wednesday, March 10, 2021 11:53 PM
> 
> On Wed, Mar 10, 2021 at 10:49:42AM -0800, Stephen Hemminger wrote:
> > On Tue,  9 Mar 2021 22:41:06 -0800
> > Tyler Retzlaff <roretzla@linux.microsoft.com> wrote:
> >
> > > based on the original commit and the usage of rte_bsf64 it appears
> the
> > > function should always have returned uint32_t instead of int which
> is
> > > consistent with the cast introduced in the return statement.
> > >
> > > Fixes: 4e261f551986 ("eal: add 64-bit bsf and 32-bit safe bsf
> > > functions")
> > > Cc: anatoly.burakov@intel.com
> > >
> > > Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> > > ---
> > >  lib/librte_eal/include/rte_common.h | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/lib/librte_eal/include/rte_common.h
> b/lib/librte_eal/include/rte_common.h
> > > index 1b630baf1..5e70ee7a8 100644
> > > --- a/lib/librte_eal/include/rte_common.h
> > > +++ b/lib/librte_eal/include/rte_common.h
> > > @@ -679,7 +679,7 @@ rte_fls_u32(uint32_t x)
> > >   * @return
> > >   *     least significant set bit in the input parameter.
> > >   */
> > > -static inline int
> > > +static inline uint32_t
> > >  rte_bsf64(uint64_t v)
> > >  {
> > >  	return (uint32_t)__builtin_ctzll(v);
> >
> > The cast is no longer needed, it should be removed.
> 
> it's not so much about making it compile. it's about making it correct
> with respect to original author intent, consistent with the rte_bsf32
> declaration, other consumers of the inline function inside rte_common.h
> and whether or not it makes sense to have a function that returns a
> count of bits signed. based on those factors i'm asserting that the
> cast is actually correct and it is the return type that is wrong.
> 
> your suggestion however would avoid having to deal with the downside of
> changing the return type which is that an api change is necessary since
> the function is exposed to applications. but even for something small
> like this i think it is best to pursue the correct change rather than
> sprinkle casts to (uint32_t) at various call-sites.
> 
> i'm in the process of sending the proposal to deprecate/change the
> return type unless others feel the above evaluation missed the mark.
> 
> thanks!

Please also update the similar math functions in rte_common.h, so the return type is consistent across these functions:
- rte_bsf32()
- rte_bsf32_safe()
- rte_fls_u32()
- rte_bsf64()
- rte_fls_u64()
- rte_log2_u32()
- rte_log2_u64()

They should all return either int or uint32_t.

Standard C conventions would have them all return int (probably due to C's default type promotion to int when used in calculations), which is also the type returned by their underlying implementation.

For some unknown reason, DPDK often uses uint32_t where you would normally use int. I guess it was inspired by MISRA C (for embedded systems); but it is not a documented conventions, and often deviated from.

I don't have a personal preference for int or uint32_t here. But at least follow the same convention in the same header file.

(Please note that the functions returning a Boolean value as an int type should keep doing that.)


Med venlig hilsen / kind regards
- Morten Brørup