[U-Boot] [PATCH v6 03/23] Add abs() macro to return absolute value

[Albert ARIBAUD <albert.u.boot@aribaud.net>]

Hi Simon,

Le 04/05/2012 20:25, Simon Glass a ?crit :
> This macro is generally useful to make it available in common.
>
> Signed-off-by: Simon Glass<sjg@chromium.org>
> ---
> Changes in v3:
> - Add new patch to put abs() in common.h
>
> Changes in v6:
> - Update x86emu and omap4 to use the abs() macro
>
>   arch/arm/cpu/armv7/omap4/clocks.c       |    2 --
>   drivers/bios_emulator/x86emu/prim_ops.c |    5 -----
>   include/common.h                        |   13 +++++++++++++
>   3 files changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/arch/arm/cpu/armv7/omap4/clocks.c b/arch/arm/cpu/armv7/omap4/clocks.c
> index e2189f7..ce3f59c 100644
> --- a/arch/arm/cpu/armv7/omap4/clocks.c
> +++ b/arch/arm/cpu/armv7/omap4/clocks.c
> @@ -46,8 +46,6 @@
>   #define puts(s)
>   #endif
>
> -#define abs(x) (((x)<  0) ? ((x)*-1) : (x))
> -
>   struct omap4_prcm_regs *const prcm = (struct omap4_prcm_regs *)0x4A004100;
>
>   const u32 sys_clk_array[8] = {
> diff --git a/drivers/bios_emulator/x86emu/prim_ops.c b/drivers/bios_emulator/x86emu/prim_ops.c
> index 7553087..5f6c795 100644
> --- a/drivers/bios_emulator/x86emu/prim_ops.c
> +++ b/drivers/bios_emulator/x86emu/prim_ops.c
> @@ -118,11 +118,6 @@ static u32 x86emu_parity_tab[8] =
>
>   #define PARITY(x)   (((x86emu_parity_tab[(x) / 32]>>  ((x) % 32))&  1) == 0)
>   #define XOR2(x)	    (((x) ^ ((x)>>1))&  0x1)
> -/*----------------------------- Implementation ----------------------------*/
> -int abs(int v)
> -{
> -	return (v>0)?v:-v;
> -}
>
>   /*----------------------------- Implementation ----------------------------*/
>
> diff --git a/include/common.h b/include/common.h
> index 74d9704..92eac2c 100644
> --- a/include/common.h
> +++ b/include/common.h
> @@ -229,6 +229,19 @@ ulong timer_get_boot_us(void);
>   #define MIN(x, y)  min(x, y)
>   #define MAX(x, y)  max(x, y)
>
> +/*
> + * Return the absolute value of a number. This handles unsigned ints, shorts
> + * and chars and returns a signed long.
> + */

What is the rationale for forcing a signed long return type? Such macros 
as max, min, abs etc usually try to avoid any forced typing. Can't you 
get rid of the ret variable and use __x directly as the return value?
+#define abs(x) ({					\
> +		long ret;				\
> +		{					\
> +			typeof((x)) __x = (x);		\
> +			ret = (__x<  0) ? -__x : __x;	\
> +		}					\
> +		ret;					\
> +	})
> +
>   #if defined(CONFIG_ENV_IS_EMBEDDED)
>   #define TOTAL_MALLOC_LEN	CONFIG_SYS_MALLOC_LEN
>   #elif ( ((CONFIG_ENV_ADDR+CONFIG_ENV_SIZE)<  CONFIG_SYS_MONITOR_BASE) || \


Amicalement,
-- 
Albert.