[PATCH] support: Add TEST_COMPARE macro

[Florian Weimer]

On 12/01/2017 08:05 PM, Paul Eggert wrote:
> 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.

I was worried about confusing diagnostics output.  But after thinking 
about it, it will not matter.

> 
>> +Â Â Â  _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.

I made the change in the attached patch.

>> +Â Â Â  /* 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.

I moved the integer promotions to the start of the macro.  As a result, 
it now works with bit fields, too.  I adjusted the comments and variable 
names further down, too.  Please have a look and tell me if this 
addresses your concerns.

(I still think that a pure run-time check is conceptually simpler, 
covers more cases, and is a reasonable compromise for a test case.)

> 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.

This should work now.

Thanks,
Florian
-------------- next part --------------
A non-text attachment was scrubbed...
Name: test_compare.patch
Type: text/x-patch
Size: 11119 bytes
Desc: not available
URL: <http://sourceware.org/pipermail/libc-alpha/attachments/20171204/f680d312/attachment.bin>