[Xcb] c_client.py: enumerator values generation

[Christian Linhart]

Hi Evgeny and all,

Thank you for your patch, and for bringing this issue to our attention.
However I am pretty sure we have to solve this problem in a different way.

Reasons:        

1. Your proposed change is in xcb/proto which is also used by generators
   for other programming languages.

   They might create wrong code because the negative number may
   really be interpreted in a wrong way in languages other than
   C/C++.

   Language specific stuff has to go into xcb/libxcb for C/C++  
   ( or xcb/xpyb for Python, etc or whatever ).

2. I have a bad gut-feeling even for C/C++.
   I am pretty sure that there are compilers which don't
   interpret this negative number as the proper bit-pattern,
   i.e., a 32bit value with bit 31 set.

Here's an alternative proposal for a solution:

I think that the core problem is that we abuse enums for things
that they weren't designed to, when we use them for masks.   

So, instead of:

  typedef enum xcb_input_modifier_mask_t {
      XCB_INPUT_MODIFIER_MASK_ANY = 2147483648
  } xcb_input_modifier_mask_t;

we should use something like:  

  typedef uint32_t xcb_input_modifier_mask_t;
  static const xcb_input_modifier_mask_t XCB_INPUT_MODIFIER_MASK_ANY = 2147483648;

Instead of static const we also could use a macro if static const is not yet supported by all relevant compilers.
(for C++ we could use "enum class", but that's another story: we don't have a generator for C++ yet) 

Maybe we should output the constants in masks as hex, and use the proper suffix, so that we get a 
32-bit unsigned constant instead of a 64 bit signed constant:

  typedef uint32_t xcb_input_modifier_mask_t;
  static const xcb_input_modifier_mask_t XCB_INPUT_MODIFIER_MASK_ANY = 0x80000000U;

This would also make these constants somehow human readable.

What do you think?

Will switching from "enum" to "typedef" and "static const" cause any ABI/API issues?
I am not aware of any but maybe I overlook something.

Cheers,

Chris

P.S.: 
This proposal would also allow us to define 64bit masks because we'd have control over the type, e.g.:

  typedef uint64_t xcb_foo_bar_mask_t;
  static const xcb_foo_bar_mask_t = XCB_FOO_BAR_BAZ 0x1ULL;
  ...
  static const xcb_foo_bar_mask_t = XCB_FOO_BAR_ABC 0x8000000000000000ULL;

On 2017-01-17 09:17, Litvinenko, Evgeny wrote:
> From 2a9446c38f7f1977b32f85dcf4857db8c676799c Mon Sep 17 00:00:00 2001
> From: Evgeny Litvinenko <evgeny.v.litvinenko at gmail.com>
> Date: Tue, 17 Jan 2017 09:32:27 +0300
> Subject: [PATCH] Convert enumerator values to the range of C 'int'
>
> This fixes warning like the following (when gcc runs with -pedantic-errors)
>
> In file included from xinput.c:14:0:
> xinput.h:3079:35: error: ISO C restricts enumerator values to range of 'int' [-Wpedantic]
>      XCB_INPUT_MODIFIER_MASK_ANY = 2147483648
>                                    ^
>
> In C99 standard (ISO/IEC 9899:1999)
> ...
> 6.7.2.2 Enumeration specifiers
> The expression that defines the value of an enumeration constant
> shall be an integer constant expression
> that has a value representable as an int.
> ...
> ---
> You can use this patch or any parts of it
> as you consider it necessary.
>
>  xcbgen/xtypes.py | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/xcbgen/xtypes.py b/xcbgen/xtypes.py
> index b83b119..93db2ad 100644
> --- a/xcbgen/xtypes.py
> +++ b/xcbgen/xtypes.py
> @@ -3,6 +3,7 @@ This module contains the classes which represent XCB data types.
>  '''
>  from xcbgen.expr import Field, Expression
>  from xcbgen.align import Alignment, AlignmentLog
> +from ctypes import c_int
>  import __main__
>
>  verbose_align_log = False
> @@ -243,7 +244,7 @@ class Enum(SimpleType):
>              if value.tag == 'value':
>                  self.values.append((item.get('name'), value.text))
>              elif value.tag == 'bit':
> -                self.values.append((item.get('name'), '%u' % (1 << int(value.text, 0))))
> +                self.values.append((item.get('name'), '%i' % c_int(1 <<  int(value.text, 0)).value))
>                  self.bits.append((item.get('name'), value.text))
>
>      def resolve(self, module):
> --
> 2.11.0
>
>
>
> Thanks,
> Evgeny.
>
> ________________________________
>
> This e-mail and any attachment(s) are intended only for the recipient(s) named above and others who have been specifically authorized to receive them. They may contain confidential information. If you are not the intended recipient, please do not read this email or its attachment(s). Furthermore, you are hereby notified that any dissemination, distribution or copying of this e-mail and any attachment(s) is strictly prohibited. If you have received this e-mail in error, please immediately notify the sender by replying to this e-mail and then delete this e-mail and any attachment(s) or copies thereof from your system. Thank you.
> _______________________________________________
> Xcb mailing list
> Xcb at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/xcb