Re: [PATCH] support: Add TEST_COMPARE macro

[Paul Eggert <eggert at cs dot ucla dot edu>]

On 12/01/2017 07:53 AM, Florian Weimer wrote:
> +/* Compare the two integers LEFT and RIGHT and report failure if they
> +   are different.  */
> +#define TEST_COMPARE(left, right)                                       \
> +  ({                                                                    \
> +    typedef __typeof__ (left) __left_type;                              \
> +    typedef __typeof__ (right) __right_type;                            \
> +    __left_type __left_value = (left);                                  \
> +    __right_type __right_value = (right);                               \
> +    /* Prevent use with floating-point and boolean types.  */           \
Why prevent booleans? They don't have problems when compared as integers.

> +    _Static_assert ((__left_type) 0.1 == (__left_type) 0,               \
> +                    "left value has floating-point type");              \
I suggest changing this to something like "(__left_type) 1.5 == 
(__left_type) 1", so that boolean types are allowed, and so that the 
expression matches the string.

> +    /* Make sure that type promotion does not alter the sign.   */      \
> +    enum                                                                \
> +    {                                                                   \
> +      __left_is_unsigned = (__left_type) -1 > 0,                        \
> +      __right_is_unsigned = (__right_type) -1 > 0,                      \
> +      __left_promotes_to_right = __left_is_unsigned                     \
> +        && sizeof (__left_value) < sizeof (__right_value),              \
> +      __right_promotes_to_left = __right_is_unsigned                    \
> +        && sizeof (__right_value) < sizeof (__left_value)               \
> +    };                                                                  \
> +    _Static_assert (__left_is_unsigned == __right_is_unsigned           \
> +                    || __left_promotes_to_right                         \
> +                    || __right_promotes_to_left,                        \
> +                    "integer promotion may alter sign of operands");    \
This confuses integer promotion (which always converts to int or to 
unsigned) with the usual integer conversions (which can convert to a 
type wider than int or unsigned int). Integer promotion does not cause 
any problems that TEST_COMPARE is trying to detect; on the contrary, 
promotion helps to prevent such problems.

For example, if __left_value is signed char and __right_value is 
unsigned short and unsigned short is narrower than int, the static 
assertion will fail even though the comparison is safe because both 
operands are promoted to int before comparison.

Please see the proposal in 
<https://sourceware.org/ml/libc-alpha/2017-11/msg00903.html>, which 
should do the right thing for such cases.