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

["Kinsella, Ray" <mdr@ashroe.eu>]

On 12/03/2021 07:34, Morten Brørup wrote:
> CC: ABI Policy maintainers. You might have an opinion. Or not. :-)

My 2c is that this is affecting inlined functions from include/rte_common.h.
Although not ideal, the practical effect on Binary Compatibility is therefore likely to be _nil_.

Just a small comment - when Steve points out "The cast is no longer needed, it should be removed."
I think Tyler _may_ have mis-understood his point when he said 

"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 ... "

My interpretation of what Steve said, is that once you change the return type
to uint32_t ... there is no need for the cast any more, that is all. 

> 
>> 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
>