Re: [lwip-devel] LWIP_U32_DIFF is unnecessarily complex

[Mason]

Mason wrote:

> src/include/lwip/def.h defines LWIP_U32_DIFF
> a macro which used to compute the difference between two
> u32_t values, taking wrap-around into account.
> 
> /** Get the absolute difference between 2 u32_t values (correcting overflows)
>  * 'a' is expected to be 'higher' (without overflow) than 'b'. */
> #define LWIP_U32_DIFF(a, b) (((a) >= (b)) ? ((a) - (b)) : (((a) + ((b) ^ 
> 0xFFFFFFFF) + 1)))
> 
> Given a 32-bit unsigned value, C guarantees that the
> expression ((b) ^ 0xFFFFFFFF) + 1 is always equal to
> the expression -b.
> 
> Let b' = b ^ 0xFFFFFFFF then b + b' = 0xFFFFFFFF
> b + b' + 1 = 0 (mod 2^32)
> b' + 1 = -b (mod 2^32)
> 
> You can also run the following program:
> 
> #include <stdio.h>
> int main(void)
> {
>   volatile unsigned b = 0;
>   do
>   {
>     unsigned v1 = (b ^ 0xFFFFFFFF) + 1;
>     unsigned v2 = -b;
>     if (v1 != v2) printf("b=%u v1=%u v2=%u\n", b, v1, v2);
>   }
>   while (++b != 0);
>   return 0;
> }
> 
> (The volatile qualifier prevents some compilers from
> optimizing the entire loop away.)
> 
> Therefore LWIP_U32_DIFF reduces to
> 
> #define LWIP_U32_DIFF(a, b) ((a)-(b))
> 
> I suggest removing the macro altogether, and applying the
> following patch.
> 
> --- timers.c.orig       2011-05-06 10:51:25.000000000 +0200
> +++ timers.c    2011-08-24 14:22:08.218750000 +0200
> @@ -365,7 +365,7 @@
>    now = sys_now();
>    if (next_timeout) {
>      /* this cares for wraparounds */
> -    diff = LWIP_U32_DIFF(now, timeouts_last_time);
> +    diff = now - timeouts_last_time;
>      do
>      {
>        had_one = 0;

Do you (devs) agree/disagree with the proposed patch?

-- 
Regards.