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

[Tyler Retzlaff <roretzla@linux.microsoft.com>]

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!