[Xcb] [PATCH] c_client.py: Prefer bit shifts over values in enum

[Peter Harris]

On 2014-07-29 18:47, Ran Benita wrote:
> Thanks for the explanations. But bear with me a little more :)
> 
> On Tue, Jul 29, 2014 at 05:43:53PM -0400, Peter Harris wrote:
>> On 2014-07-29 17:14, Ran Benita wrote:
>>> On Tue, Jul 29, 2014 at 04:59:12PM -0400, Peter Harris wrote:
>>>> On 2014-07-29 16:26, Daniel Martin wrote:
>>>>> Any idea or wish how I should change the patch, how the resulting value
>>>>> should like?
>>>>
>>>> 0x80000000 simply isn't representable in an enum in standard C.
>>>
>>> Can you explain how this patch makes things worse in this regard?
>>
>> It's signed overflow. (1 << 31) is an integer greater than INT_MAX.
> 
> But the current constant also cannot be represented in an int. So I'm
> not sure how (1u << 31u) is worse?

(1u << 31u) is better than (1 << 31) because it's not a calculation
causing a signed overflow. It is not significantly better than the
current 2147483648, and it is exactly equivalent to 2147483648u.

(1u << 31u) is worse than (1 << 31), because it throws the same warning
as the current large constant integer.

>> Note also https://gcc.gnu.org/wiki/FAQ#signed_overflow
>>
>>> (You mentioned a gcc extension, which one?).
>>
>> https://gcc.gnu.org/onlinedocs/gcc/Integers-implementation.html#Integers-implementation
>>
>> Specifically, the two bullet points "The result of, or the signal raised
>> by, converting an integer to a signed integer type when the value cannot
>> be represented in an object of that type"
> 
> Again, what's the difference from the current status?

There isn't one.

>> and "The results of some
>> bitwise operations on signed integers".
>>
>> In particular, "GCC does not use the latitude given in C99 and C11 only
>> to treat certain aspects of signed ‘<<’ as undefined, but this is
>> subject to change." Where 1<<31 appears to be one of the "certain
>> aspects", if I read the spec correctly.
> 
> If we use (1u << 31u), this does not apply, no?

Correct.

> You get an unsigned
> constant, which needs to be coverable to an int, and you hit the
> paragraph above.

Correct.

We're between a rock and a hard place here. What we really want is an
unsigned value that isn't assigned to an enum (because enums can't be
relied upon to hold values outside of the range of 'int').

If we accept the constraint that it must be assigned to an enum, is it
better to rely on an implementation defined (unsigned to signed)
conversion, or an implementation defined (signed bitshift out of range)?

I argue that (unsigned to signed) is less likely to surprise than
(signed bitshift out of range), especially in light of an explicit "this
is subject to change" about (signed bitshift out of range) from GCC.

On the other side of the argument, (signed bitshift out of range)
doesn't trigger -Wall -pedantic for some reason.

My position is that dropping the patch and putting up with the warning
is better than accepting the patch, hiding the warning, and relying on
implementation-defined behaviour that is subject to change.

If we do not accept the constraint that it must be assigned to an enum,
changing values out of range for an int from an enum to a #define is a
much better patch. But we must be willing to accept the chance that some
user of the value may get a new warning if they assign it to a variable
of type enum (instead of a variable of type uint32_t).

Peter Harris
-- 
               Open Text Connectivity Solutions Group
Peter Harris                    http://connectivity.opentext.com/
Research and Development        Phone: +1 905 762 6001
pharris at opentext.com            Toll Free: 1 877 359 4866