[2/3] eal: change rte_fls and rte_bsf to return uint32_t

Message ID 1659993692-17479-3-git-send-email-roretzla@linux.microsoft.com (mailing list archive)
State Accepted, archived
Delegated to: Thomas Monjalon
Headers
Series cleanup bsf and fls inline function return types |

Checks

Context Check Description
ci/checkpatch warning coding style issues

Commit Message

Tyler Retzlaff Aug. 8, 2022, 9:21 p.m. UTC
  From: Tyler Retzlaff <roretzla@microsoft.com>

return fixed width uint32_t to be consistent with what appears to
be the original authors intent. it doesn't make much sense to return
signed integers for these functions.

Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
---
 lib/eal/include/rte_common.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
  

Comments

Thomas Monjalon Oct. 5, 2022, 9:02 a.m. UTC | #1
08/08/2022 23:21, Tyler Retzlaff:
> From: Tyler Retzlaff <roretzla@microsoft.com>
> 
> return fixed width uint32_t to be consistent with what appears to
> be the original authors intent. it doesn't make much sense to return
> signed integers for these functions.
> 
> Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> ---
>  lib/eal/include/rte_common.h | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/eal/include/rte_common.h b/lib/eal/include/rte_common.h
> index a96cc2a..bd4184d 100644
> --- a/lib/eal/include/rte_common.h
> +++ b/lib/eal/include/rte_common.h
> @@ -707,7 +707,7 @@ static void __attribute__((destructor(RTE_PRIO(prio)), used)) func(void)
>   * @return
>   *     The last (most-significant) bit set, or 0 if the input is 0.
>   */
> -static inline int
> +static inline uint32_t
>  rte_fls_u32(uint32_t x)
>  {
>  	return (x == 0) ? 0 : 32 - __builtin_clz(x);
> @@ -724,7 +724,7 @@ static void __attribute__((destructor(RTE_PRIO(prio)), used)) func(void)
>   * @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);
> @@ -766,7 +766,7 @@ static void __attribute__((destructor(RTE_PRIO(prio)), used)) func(void)
>   * @return
>   *     The last (most-significant) bit set, or 0 if the input is 0.
>   */
> -static inline int
> +static inline uint32_t
>  rte_fls_u64(uint64_t x)
>  {
>  	return (x == 0) ? 0 : 64 - __builtin_clzll(x);
> 

You forgot the _safe versions:

--- a/lib/eal/include/rte_common.h
+++ b/lib/eal/include/rte_common.h
@@ -660,7 +660,7 @@ rte_bsf32(uint32_t v)
  * @return
  *     Returns 0 if ``v`` was 0, otherwise returns 1.
  */
-static inline int
+static inline uint32_t
 rte_bsf32_safe(uint32_t v, uint32_t *pos)
 {
        if (v == 0)
@@ -739,7 +739,7 @@ rte_bsf64(uint64_t v)
  * @return
  *     Returns 0 if ``v`` was 0, otherwise returns 1.
  */
-static inline int
+static inline uint32_t
 rte_bsf64_safe(uint64_t v, uint32_t *pos)
 {
        if (v == 0)
  
Tyler Retzlaff Oct. 5, 2022, 3:15 p.m. UTC | #2
On Wed, Oct 05, 2022 at 11:02:45AM +0200, Thomas Monjalon wrote:
> 08/08/2022 23:21, Tyler Retzlaff:
> > From: Tyler Retzlaff <roretzla@microsoft.com>
> > 
> 
> You forgot the _safe versions:

> 
> --- a/lib/eal/include/rte_common.h
> +++ b/lib/eal/include/rte_common.h
> @@ -660,7 +660,7 @@ rte_bsf32(uint32_t v)
>   * @return
>   *     Returns 0 if ``v`` was 0, otherwise returns 1.
>   */
> -static inline int
> +static inline uint32_t
>  rte_bsf32_safe(uint32_t v, uint32_t *pos)
>  {
>         if (v == 0)
> @@ -739,7 +739,7 @@ rte_bsf64(uint64_t v)
>   * @return
>   *     Returns 0 if ``v`` was 0, otherwise returns 1.
>   */
> -static inline int
> +static inline uint32_t
>  rte_bsf64_safe(uint64_t v, uint32_t *pos)
>  {
>         if (v == 0)
> 
> 
> 
> 

the return values from the _safe versions are `int' treated
like a bool type. they have been left as is to be consistent
with the rest of dpdk return value types.

the non-safe version returns were returning actual values
and not an indication of success or failure.

they could certainly be changed to C99 fixed width types but
if they are changed at all perhaps they should be changed to
_Bool or bool from stdbool.h?

it looks like this change has been merged already but if you
would like to make any further changes let me know i'll take
care of it.

thanks
  
Thomas Monjalon Oct. 5, 2022, 3:23 p.m. UTC | #3
05/10/2022 17:15, Tyler Retzlaff:
> On Wed, Oct 05, 2022 at 11:02:45AM +0200, Thomas Monjalon wrote:
> > 08/08/2022 23:21, Tyler Retzlaff:
> > > From: Tyler Retzlaff <roretzla@microsoft.com>
> > > 
> > 
> > You forgot the _safe versions:
> 
> > 
> > --- a/lib/eal/include/rte_common.h
> > +++ b/lib/eal/include/rte_common.h
> > @@ -660,7 +660,7 @@ rte_bsf32(uint32_t v)
> >   * @return
> >   *     Returns 0 if ``v`` was 0, otherwise returns 1.
> >   */
> > -static inline int
> > +static inline uint32_t
> >  rte_bsf32_safe(uint32_t v, uint32_t *pos)
> >  {
> >         if (v == 0)
> > @@ -739,7 +739,7 @@ rte_bsf64(uint64_t v)
> >   * @return
> >   *     Returns 0 if ``v`` was 0, otherwise returns 1.
> >   */
> > -static inline int
> > +static inline uint32_t
> >  rte_bsf64_safe(uint64_t v, uint32_t *pos)
> >  {
> >         if (v == 0)
> > 
> > 
> > 
> > 
> 
> the return values from the _safe versions are `int' treated
> like a bool type. they have been left as is to be consistent
> with the rest of dpdk return value types.
> 
> the non-safe version returns were returning actual values
> and not an indication of success or failure.
> 
> they could certainly be changed to C99 fixed width types but
> if they are changed at all perhaps they should be changed to
> _Bool or bool from stdbool.h?
> 
> it looks like this change has been merged already but if you
> would like to make any further changes let me know i'll take
> care of it.

Sorry, it's my mistake, I went too fast.
I'll revert them to int.
  

Patch

diff --git a/lib/eal/include/rte_common.h b/lib/eal/include/rte_common.h
index a96cc2a..bd4184d 100644
--- a/lib/eal/include/rte_common.h
+++ b/lib/eal/include/rte_common.h
@@ -707,7 +707,7 @@  static void __attribute__((destructor(RTE_PRIO(prio)), used)) func(void)
  * @return
  *     The last (most-significant) bit set, or 0 if the input is 0.
  */
-static inline int
+static inline uint32_t
 rte_fls_u32(uint32_t x)
 {
 	return (x == 0) ? 0 : 32 - __builtin_clz(x);
@@ -724,7 +724,7 @@  static void __attribute__((destructor(RTE_PRIO(prio)), used)) func(void)
  * @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);
@@ -766,7 +766,7 @@  static void __attribute__((destructor(RTE_PRIO(prio)), used)) func(void)
  * @return
  *     The last (most-significant) bit set, or 0 if the input is 0.
  */
-static inline int
+static inline uint32_t
 rte_fls_u64(uint64_t x)
 {
 	return (x == 0) ? 0 : 64 - __builtin_clzll(x);