[v2] c_client.py: avoid field name clash with C++ 'explicit' keyword

Submitted by Klemens Baum on Dec. 27, 2015, 9:06 p.m.

Details

Message ID 1451250387-31121-2-git-send-email-klemens.baum@gmail.com
State New
Series "c_client.py: avoid field name clash with C++ 'explicit' keyword"
Headers show

Commit Message

Klemens Baum Dec. 27, 2015, 9:06 p.m.
The generated code was causing compilation errors when xcb/xkb.h
is included in a C++ code base.

To avoid breaking the API for C users, the unprefixed field names
are provided as alternative names via an anonymous union.

For existing C++ users, the relevant header file would not have
compiled in the first place, so breaking the API is justified.
If they were abusing the preprocessor to modify the header,
it's their own fault for not resolving the issue upstream.

This resolves bug #74080.
---
 src/c_client.py | 74 ++++++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 50 insertions(+), 24 deletions(-)

--
2.6.4

Patch hide | download patch | download mbox

diff --git a/src/c_client.py b/src/c_client.py
index c38b434..343e9d6 100644
--- a/src/c_client.py
+++ b/src/c_client.py
@@ -17,9 +17,10 @@  _cname_special_cases = {'DECnet':'decnet'}

 _extension_special_cases = ['XPrint', 'XCMisc', 'BigRequests']

-_cplusplus_annoyances = {'class' : '_class',
-                         'new'   : '_new',
-                         'delete': '_delete'}
+_cplusplus_annoyances = {'class'   : '_class',
+                         'new'     : '_new',
+                         'delete'  : '_delete',
+                         'explicit': '_explicit'}
 _c_keywords = {'default' : '_default'}

 _hlines = []
@@ -192,16 +193,21 @@  def _n_item(str):
         name_parts = [match.group(0) for match in split]
         return '_'.join(name_parts)

-def _cpp(str):
+def _c_sanitize(str):
+    '''
+    Checks for certain C reserved words and fixes them.
+    '''
+    if str in _c_keywords:
+        return  _c_keywords[str]
+    return str
+
+def _cpp_sanitize(str):
     '''
     Checks for certain C++ reserved words and fixes them.
     '''
     if str in _cplusplus_annoyances:
         return _cplusplus_annoyances[str]
-    elif str in _c_keywords:
-        return  _c_keywords[str]
-    else:
-        return str
+    return _c_sanitize(str)

 def _ext(str):
     '''
@@ -297,6 +303,14 @@  def c_open(self):
     _h('')
     _h('#ifdef __cplusplus')
     _h('extern "C" {')
+    _h('#define XCB_ALTERNATIVE_NAMES(type, c_name, cpp_name) \\')
+    _h('    type cpp_name')
+    _h('#else')
+    _h('#define XCB_ALTERNATIVE_NAMES(type, c_name, cpp_name) \\')
+    _h('    union {                                           \\')
+    _h('        type c_name;                                  \\')
+    _h('        type cpp_name;                                \\')
+    _h('    }')
     _h('#endif')

     if _ns.is_ext:
@@ -324,6 +338,9 @@  def c_close(self):
     _h('#endif')

     _h('')
+    _h('#undef XCB_ALTERNATIVE_NAMES')
+
+    _h('')
     _h('#endif')
     _h('')
     _h('/**')
@@ -425,7 +442,8 @@  def _c_type_setup(self, name, postfix):
         self.c_need_serialize = True
         self.c_container = 'struct'
         for bitcase in self.bitcases:
-            bitcase.c_field_name = _cpp(bitcase.field_name)
+            bitcase.c_field_name = _cpp_sanitize(bitcase.field_name)
+            bitcase.c_legacy_field_name = _c_sanitize(bitcase.field_name)
             bitcase_name = bitcase.field_type if bitcase.type.has_name else name
             _c_type_setup(bitcase.type, bitcase_name, ())

@@ -439,7 +457,8 @@  def _c_type_setup(self, name, postfix):
         for field in self.fields:
             field.c_field_type = _t(field.field_type)
             field.c_field_const_type = ('' if field.type.nmemb == 1 else 'const ') + field.c_field_type
-            field.c_field_name = _cpp(field.field_name)
+            field.c_field_name = _cpp_sanitize(field.field_name)
+            field.c_legacy_field_name = _c_sanitize(field.field_name)
             field.c_subscript = '[%d]' % field.type.nmemb if (field.type.nmemb and field.type.nmemb > 1) else ''
             field.c_pointer = ' ' if field.type.nmemb == 1 else '*'

@@ -580,13 +599,13 @@  def _c_helper_fieldaccess_expr(prefix, field=None):
                 # (also, their accessor function needs a different arglist
                 # so this would require special treatment here)
                 # Therefore: Access as struct member
-                prefix_str += last_sep + _cpp(field.field_name)
+                prefix_str += last_sep + _cpp_sanitize(field.field_name)
             else:
                 # Access with the accessor function
                 prefix_str = field.c_accessor_name + "(" + prefix_str + ")"
         else:
             # Access as struct member
-            prefix_str += last_sep + _cpp(field.field_name)
+            prefix_str += last_sep + _cpp_sanitize(field.field_name)

     return prefix_str

@@ -612,7 +631,7 @@  def _c_helper_field_mapping(complex_type, prefix, flat=False):
             if f.field_name in all_fields:
                 raise Exception("field name %s has been registered before" % f.field_name)

-            all_fields[f.field_name] = (fname, f)
+            all_fields[f.c_field_name] = (fname, f)
             if f.type.is_container and not flat:
                 if f.type.is_case_or_bitcase and not f.type.has_name:
                     new_prefix = prefix
@@ -1073,9 +1092,9 @@  def _c_serialize_helper_fields_fixed_size(context, self, field,
                 raise Exception("type for field '%s' (expression '%s') unkown" %
                                 (field.field_name, _c_accessor_get_expr(field.type.expr)))

-            temp_vars.append('    %s xcb_expr_%s = %s;' % (field.type.c_type, _cpp(field.field_name),
+            temp_vars.append('    %s xcb_expr_%s = %s;' % (field.type.c_type, _cpp_sanitize(field.field_name),
                                                            _c_accessor_get_expr(field.type.expr, prefix)))
-            value += "&xcb_expr_%s;" % _cpp(field.field_name)
+            value += "&xcb_expr_%s;" % _cpp_sanitize(field.field_name)

         elif field.type.is_pad:
             if field.type.nmemb == 1:
@@ -2054,6 +2073,12 @@  def _c_complex(self, force_packed = False):
     struct_fields = []
     maxtypelen = 0

+    def _declare_as_pointer(self, field):
+        return not ((field.type.fixed_size() and not self.is_union) or
+            # in case of switch with switch children, don't make the field a pointer
+            # necessary for unserialize to work
+            (self.is_switch and field.type.is_switch))
+
     for field in self.fields:
         if field.wire and (field.type.fixed_size() or self.is_switch or self.is_union):
             struct_fields.append(field)
@@ -2061,20 +2086,21 @@  def _c_complex(self, force_packed = False):
     for field in struct_fields:
         length = len(field.c_field_type)
         # account for '*' pointer_spec
-        if not field.type.fixed_size() and not self.is_union:
+        if _declare_as_pointer(self, field):
             length += 1
         maxtypelen = max(maxtypelen, length)

     def _c_complex_field(self, field, space=''):
-        if (field.type.fixed_size() or self.is_union or
-            # in case of switch with switch children, don't make the field a pointer
-            # necessary for unserialize to work
-            (self.is_switch and field.type.is_switch)):
-            spacing = ' ' * (maxtypelen - len(field.c_field_type))
-            _h('%s    %s%s %s%s;', space, field.c_field_type, spacing, field.c_field_name, field.c_subscript)
+        if field.c_field_name != field.c_legacy_field_name:
+            pointer = ' *' if _declare_as_pointer(self, field) else ''
+            _h('%s    XCB_ALTERNATIVE_NAMES(%s%s, %s, %s);', space, field.c_field_type,
+                    pointer, field.c_legacy_field_name, field.c_field_name)
         else:
-            spacing = ' ' * (maxtypelen - (len(field.c_field_type) + 1))
-            _h('%s    %s%s *%s%s;', space, field.c_field_type, spacing, field.c_field_name, field.c_subscript)
+            spacing = ' ' * (maxtypelen - len(field.c_field_type))
+            pointer = '*' if _declare_as_pointer(self, field) else ' '
+            _h('%s    %s%s%s%s%s;', space, field.c_field_type, spacing, pointer,
+                    field.c_field_name, field.c_subscript)
+

     if not self.is_switch:
         for field in struct_fields:

Comments

Keith Packard Dec. 28, 2015, 7:14 p.m.
Klemens Baum <klemensbaum@gmail.com> writes:

> The generated code was causing compilation errors when xcb/xkb.h
> is included in a C++ code base.
>
> To avoid breaking the API for C users, the unprefixed field names
> are provided as alternative names via an anonymous union.

Why not just leave the C names alone and use the new names when included
in a c++ file? You've already accepted a different API for C/C++, after all.
Ran Benita Dec. 28, 2015, 8:09 p.m.
On Sun, Dec 27, 2015 at 10:06:27PM +0100, Klemens Baum wrote:
> The generated code was causing compilation errors when xcb/xkb.h
> is included in a C++ code base.
> 
> To avoid breaking the API for C users, the unprefixed field names
> are provided as alternative names via an anonymous union.

I'm not sure if XCB is willing to use C11 already. Though I think gcc
supports anonymous unions for a long time.

> For existing C++ users, the relevant header file would not have
> compiled in the first place, so breaking the API is justified.
> If they were abusing the preprocessor to modify the header,
> it's their own fault for not resolving the issue upstream.

Note that qt already works around the issue:
https://github.com/qtproject/qtbase/blob/e979f8721731bc5f4cd0d65830548f9e70da2da5/src/plugins/platforms/xcb/qxcbconnection.h#L51

Ran

> This resolves bug #74080.
> ---
>  src/c_client.py | 74 ++++++++++++++++++++++++++++++++++++++-------------------
>  1 file changed, 50 insertions(+), 24 deletions(-)
> 
> diff --git a/src/c_client.py b/src/c_client.py
> index c38b434..343e9d6 100644
> --- a/src/c_client.py
> +++ b/src/c_client.py
> @@ -17,9 +17,10 @@ _cname_special_cases = {'DECnet':'decnet'}
> 
>  _extension_special_cases = ['XPrint', 'XCMisc', 'BigRequests']
> 
> -_cplusplus_annoyances = {'class' : '_class',
> -                         'new'   : '_new',
> -                         'delete': '_delete'}
> +_cplusplus_annoyances = {'class'   : '_class',
> +                         'new'     : '_new',
> +                         'delete'  : '_delete',
> +                         'explicit': '_explicit'}
>  _c_keywords = {'default' : '_default'}
> 
>  _hlines = []
> @@ -192,16 +193,21 @@ def _n_item(str):
>          name_parts = [match.group(0) for match in split]
>          return '_'.join(name_parts)
> 
> -def _cpp(str):
> +def _c_sanitize(str):
> +    '''
> +    Checks for certain C reserved words and fixes them.
> +    '''
> +    if str in _c_keywords:
> +        return  _c_keywords[str]
> +    return str
> +
> +def _cpp_sanitize(str):
>      '''
>      Checks for certain C++ reserved words and fixes them.
>      '''
>      if str in _cplusplus_annoyances:
>          return _cplusplus_annoyances[str]
> -    elif str in _c_keywords:
> -        return  _c_keywords[str]
> -    else:
> -        return str
> +    return _c_sanitize(str)
> 
>  def _ext(str):
>      '''
> @@ -297,6 +303,14 @@ def c_open(self):
>      _h('')
>      _h('#ifdef __cplusplus')
>      _h('extern "C" {')
> +    _h('#define XCB_ALTERNATIVE_NAMES(type, c_name, cpp_name) \\')
> +    _h('    type cpp_name')
> +    _h('#else')
> +    _h('#define XCB_ALTERNATIVE_NAMES(type, c_name, cpp_name) \\')
> +    _h('    union {                                           \\')
> +    _h('        type c_name;                                  \\')
> +    _h('        type cpp_name;                                \\')
> +    _h('    }')
>      _h('#endif')
> 
>      if _ns.is_ext:
> @@ -324,6 +338,9 @@ def c_close(self):
>      _h('#endif')
> 
>      _h('')
> +    _h('#undef XCB_ALTERNATIVE_NAMES')
> +
> +    _h('')
>      _h('#endif')
>      _h('')
>      _h('/**')
> @@ -425,7 +442,8 @@ def _c_type_setup(self, name, postfix):
>          self.c_need_serialize = True
>          self.c_container = 'struct'
>          for bitcase in self.bitcases:
> -            bitcase.c_field_name = _cpp(bitcase.field_name)
> +            bitcase.c_field_name = _cpp_sanitize(bitcase.field_name)
> +            bitcase.c_legacy_field_name = _c_sanitize(bitcase.field_name)
>              bitcase_name = bitcase.field_type if bitcase.type.has_name else name
>              _c_type_setup(bitcase.type, bitcase_name, ())
> 
> @@ -439,7 +457,8 @@ def _c_type_setup(self, name, postfix):
>          for field in self.fields:
>              field.c_field_type = _t(field.field_type)
>              field.c_field_const_type = ('' if field.type.nmemb == 1 else 'const ') + field.c_field_type
> -            field.c_field_name = _cpp(field.field_name)
> +            field.c_field_name = _cpp_sanitize(field.field_name)
> +            field.c_legacy_field_name = _c_sanitize(field.field_name)
>              field.c_subscript = '[%d]' % field.type.nmemb if (field.type.nmemb and field.type.nmemb > 1) else ''
>              field.c_pointer = ' ' if field.type.nmemb == 1 else '*'
> 
> @@ -580,13 +599,13 @@ def _c_helper_fieldaccess_expr(prefix, field=None):
>                  # (also, their accessor function needs a different arglist
>                  # so this would require special treatment here)
>                  # Therefore: Access as struct member
> -                prefix_str += last_sep + _cpp(field.field_name)
> +                prefix_str += last_sep + _cpp_sanitize(field.field_name)
>              else:
>                  # Access with the accessor function
>                  prefix_str = field.c_accessor_name + "(" + prefix_str + ")"
>          else:
>              # Access as struct member
> -            prefix_str += last_sep + _cpp(field.field_name)
> +            prefix_str += last_sep + _cpp_sanitize(field.field_name)
> 
>      return prefix_str
> 
> @@ -612,7 +631,7 @@ def _c_helper_field_mapping(complex_type, prefix, flat=False):
>              if f.field_name in all_fields:
>                  raise Exception("field name %s has been registered before" % f.field_name)
> 
> -            all_fields[f.field_name] = (fname, f)
> +            all_fields[f.c_field_name] = (fname, f)
>              if f.type.is_container and not flat:
>                  if f.type.is_case_or_bitcase and not f.type.has_name:
>                      new_prefix = prefix
> @@ -1073,9 +1092,9 @@ def _c_serialize_helper_fields_fixed_size(context, self, field,
>                  raise Exception("type for field '%s' (expression '%s') unkown" %
>                                  (field.field_name, _c_accessor_get_expr(field.type.expr)))
> 
> -            temp_vars.append('    %s xcb_expr_%s = %s;' % (field.type.c_type, _cpp(field.field_name),
> +            temp_vars.append('    %s xcb_expr_%s = %s;' % (field.type.c_type, _cpp_sanitize(field.field_name),
>                                                             _c_accessor_get_expr(field.type.expr, prefix)))
> -            value += "&xcb_expr_%s;" % _cpp(field.field_name)
> +            value += "&xcb_expr_%s;" % _cpp_sanitize(field.field_name)
> 
>          elif field.type.is_pad:
>              if field.type.nmemb == 1:
> @@ -2054,6 +2073,12 @@ def _c_complex(self, force_packed = False):
>      struct_fields = []
>      maxtypelen = 0
> 
> +    def _declare_as_pointer(self, field):
> +        return not ((field.type.fixed_size() and not self.is_union) or
> +            # in case of switch with switch children, don't make the field a pointer
> +            # necessary for unserialize to work
> +            (self.is_switch and field.type.is_switch))
> +
>      for field in self.fields:
>          if field.wire and (field.type.fixed_size() or self.is_switch or self.is_union):
>              struct_fields.append(field)
> @@ -2061,20 +2086,21 @@ def _c_complex(self, force_packed = False):
>      for field in struct_fields:
>          length = len(field.c_field_type)
>          # account for '*' pointer_spec
> -        if not field.type.fixed_size() and not self.is_union:
> +        if _declare_as_pointer(self, field):
>              length += 1
>          maxtypelen = max(maxtypelen, length)
> 
>      def _c_complex_field(self, field, space=''):
> -        if (field.type.fixed_size() or self.is_union or
> -            # in case of switch with switch children, don't make the field a pointer
> -            # necessary for unserialize to work
> -            (self.is_switch and field.type.is_switch)):
> -            spacing = ' ' * (maxtypelen - len(field.c_field_type))
> -            _h('%s    %s%s %s%s;', space, field.c_field_type, spacing, field.c_field_name, field.c_subscript)
> +        if field.c_field_name != field.c_legacy_field_name:
> +            pointer = ' *' if _declare_as_pointer(self, field) else ''
> +            _h('%s    XCB_ALTERNATIVE_NAMES(%s%s, %s, %s);', space, field.c_field_type,
> +                    pointer, field.c_legacy_field_name, field.c_field_name)
>          else:
> -            spacing = ' ' * (maxtypelen - (len(field.c_field_type) + 1))
> -            _h('%s    %s%s *%s%s;', space, field.c_field_type, spacing, field.c_field_name, field.c_subscript)
> +            spacing = ' ' * (maxtypelen - len(field.c_field_type))
> +            pointer = '*' if _declare_as_pointer(self, field) else ' '
> +            _h('%s    %s%s%s%s%s;', space, field.c_field_type, spacing, pointer,
> +                    field.c_field_name, field.c_subscript)
> +
> 
>      if not self.is_switch:
>          for field in struct_fields:
> --
> 2.6.4
> 
> _______________________________________________
> Xcb mailing list
> Xcb@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/xcb
Klemens Baum Dec. 29, 2015, 12:46 a.m.
On Mon, Dec 28, 2015 at 8:14 PM, Keith Packard <keithp@keithp.com> wrote:
>
> Why not just leave the C names alone and use the new names when included
> in a c++ file? You've already accepted a different API for C/C++, after all.

That's certainly an option, but it would mean that C code using XCB
would not compile with a C++ compiler.
It may also create confusion for users because of the inconsistency
with class, new and delete already having the underscore in the C
version but explicit only being prefixed in C++ mode.

That said, if we want to do that, it's simply a matter of changing the
definition of XCB_ALTERNATIVE_NAMES to `type c_name' in the #else part
of the '#ifdef __cplusplus` and moving the three already-prefixed
keywords from _cplusplus_annoyances to _c_keywords.
Klemens Baum Dec. 29, 2015, 1:10 a.m.
On Mon, Dec 28, 2015 at 9:09 PM, Ran Benita <ran234@gmail.com> wrote:
> I'm not sure if XCB is willing to use C11 already. Though I think gcc
> supports anonymous unions for a long time.
I didn't think of that, since C++ has had anonymous unions from the beginning.
It seems that both gcc and clang allow anonymous union in pre-C11 code
for "compatibility with other compilers." (see
https://gcc.gnu.org/onlinedocs/gcc/Unnamed-Fields.html)

> Note that qt already works around the issue:
> https://github.com/qtproject/qtbase/blob/e979f8721731bc5f4cd0d65830548f9e70da2da5/src/plugins/platforms/xcb/qxcbconnection.h#L51
They don't actually use the explicit field though, so their build will
be unaffected by this change.