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-boun...@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 <roret...@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.bura...@intel.com
>>>>
>>>> Signed-off-by: Tyler Retzlaff <roret...@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
>
