[libxcb,10/7] c_client.py: don't generate _aux() request functions

Submitted by Ran Benita on Feb. 25, 2014, 12:11 p.m.

Details

Message ID 1393330297-25151-3-git-send-email-ran234@gmail.com
State Rejected
Headers show

Not browsing as part of any series.

Commit Message

Ran Benita Feb. 25, 2014, 12:11 p.m.
Currently, for requests which take a parameter which needs to be
serialized (like some complex struct), there are two types of functions
generated:
    xbc_<request_name>{,checked,unchecked}
    xbc_<request_name>_aux{,checked,unchecked}
The difference between the aux and non-aux versions is how they handle
the complex argument(s):

- The aux version takes a type-safe instance of the struct, and
  internally calls the _serialize() function for this struct, which
  translates it to its wire representation.

- The non-aux version instead takes a void*, unpacks it to the struct in
  order to calculate the sizeof, and then sends it directly.

The non-aux version is not type-safe and expects the user to lay out the
data manually. People also don't know the aux version even exists, and
thus use the non-aux version incorrectly.

Therefore, remove the non-aux versions and drop the _aux prefix
entirely, and make the code-generator worry about one less obscurity on
the way.

The affected functions are:

xcb_sync_create_alarm
xcb_sync_change_alarm
    It was recently fixed in proto to use a switch, which caused the
    _aux function to be generated:
    http://cgit.freedesktop.org/xcb/proto/commit/?id=e6a246e50e62cbcba33d0e1d2371e69e6e089383

    It is used (incorrectly) by KDE kwin. They have added a workaround
    because it is evidently not working (should have used the aux
    version):
    https://projects.kde.org/projects/kde/kde-workspace/repository/revisions/008ac5efabfb99f04813e5dad29c2d0a92d13fc5
    In my tests it continues to not-work as before, but with this change
    they will get a type error, which would hopefully prompt them to fix
    the call.

    Finally, I wasn't able to find anyone using the _aux version, except
    this Weston patch which wasn't applied:
    http://lists.freedesktop.org/archives/wayland-devel/2013-July/010416.html

xcb_input_change_device_property
xcb_input_xi_change_property
    xinput is still disabled by default, and I'm quite sure these
    functions aren't used in any case.

xcb_xkb_select_events
    This function is used by Qt5, xkbcommon-x11 and similar others. All
    of them pass NULL to the last (aux) argument, and they continue to
    work correctly with this change.

xcb_xkb_set_map
xcb_xkb_set_names
    These functions are not used anywhere.

Cc: Louis-Francis Ratté-Boulianne <lfrb@collabora.com>
Cc: Martin Gräßlin <mgraesslin@kde.org>
Signed-off-by: Ran Benita <ran234@gmail.com>
---
 src/c_client.py | 75 ++++++++++++++++++++-------------------------------------
 1 file changed, 26 insertions(+), 49 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/c_client.py b/src/c_client.py
index f2bef2f..8690a59 100644
--- a/src/c_client.py
+++ b/src/c_client.py
@@ -305,13 +305,9 @@  def _c_type_setup(self, name, postfix):
     self.c_cookie_type = _t(name + ('cookie',))
     self.c_reply_fds_name = _n(name + ('reply_fds',))
 
-    self.c_need_aux = False
     self.c_need_serialize = False
     self.c_need_sizeof = False
 
-    self.c_aux_name = _n(name + ('aux',))
-    self.c_aux_checked_name = _n(name + ('aux', 'checked'))
-    self.c_aux_unchecked_name = _n(name + ('aux', 'unchecked'))
     self.c_serialize_name = _n(name + ('serialize',))
     self.c_unserialize_name = _n(name + ('unserialize',))
     self.c_unpack_name = _n(name + ('unpack',))
@@ -357,7 +353,6 @@  def _c_type_setup(self, name, postfix):
             if field.type.is_switch:
                 field.c_pointer = '*'
                 field.c_field_const_type = 'const ' + field.c_field_type
-                self.c_need_aux = True
             elif not field.type.fixed_size() and not field.type.is_bitcase:
                 self.c_need_sizeof = True
 
@@ -1763,7 +1758,7 @@  def c_union(self, name):
     _c_complex(self)
     _c_iterator(self, name)
 
-def _c_request_helper(self, name, cookie_type, void, regular, aux=False, reply_fds=False):
+def _c_request_helper(self, name, cookie_type, void, regular, reply_fds=False):
     '''
     Declares a request function.
     '''
@@ -1802,11 +1797,12 @@  def _c_request_helper(self, name, cookie_type, void, regular, aux=False, reply_f
     func_ext_global = '&' + _ns.c_ext_global_name if _ns.is_ext else '0'
 
     # What our function name is
-    func_name = self.c_request_name if not aux else self.c_aux_name
     if checked:
-        func_name = self.c_checked_name if not aux else self.c_aux_checked_name
-    if unchecked:
-        func_name = self.c_unchecked_name if not aux else self.c_aux_unchecked_name
+        func_name = self.c_checked_name
+    elif unchecked:
+        func_name = self.c_unchecked_name
+    else:
+        func_name = self.c_request_name
 
     param_fields = []
     wire_fields = []
@@ -1827,8 +1823,6 @@  def _c_request_helper(self, name, cookie_type, void, regular, aux=False, reply_f
 
     for field in param_fields:
         c_field_const_type = field.c_field_const_type
-        if field.type.c_need_serialize and not aux:
-            c_field_const_type = "const void"
         if len(c_field_const_type) > maxtypelen:
             maxtypelen = len(c_field_const_type)
         if field.type.is_list and not field.type.member.fixed_size():
@@ -1850,7 +1844,7 @@  def _c_request_helper(self, name, cookie_type, void, regular, aux=False, reply_f
     if hasattr(self, "doc") and self.doc:
         for field in param_fields:
             # XXX: hard-coded until we fix xproto.xml
-            base_func_name = self.c_request_name if not aux else self.c_aux_name
+            base_func_name = self.c_request_name
             if base_func_name == 'xcb_change_gc' and field.c_field_name == 'value_mask':
                 field.enum = 'GC'
             elif base_func_name == 'xcb_change_window_attributes' and field.c_field_name == 'value_mask':
@@ -1915,9 +1909,6 @@  def _c_request_helper(self, name, cookie_type, void, regular, aux=False, reply_f
         count = count - 1
         c_field_const_type = field.c_field_const_type
         c_pointer = field.c_pointer
-        if field.type.c_need_serialize and not aux:
-            c_field_const_type = "const void"
-            c_pointer = '*'
         spacing = ' ' * (maxtypelen - len(c_field_const_type))
         comma = ',' if count else ');'
         _h('%s%s%s %s%s  /**< */%s', func_spacing, c_field_const_type,
@@ -1951,9 +1942,8 @@  def _c_request_helper(self, name, cookie_type, void, regular, aux=False, reply_f
         _c('    /* in the protocol description, variable size fields are followed by fixed size fields */')
         _c('    void *xcb_aux = 0;')
 
-
-    for idx, f in enumerate(serial_fields):
-        if aux:
+    for idx, field in enumerate(serial_fields):
+        if field.type.c_need_serialize:
             _c('    void *xcb_aux%d = 0;' % (idx))
     if list_with_var_size_elems:
         _c('    unsigned int i;')
@@ -1979,7 +1969,7 @@  def _c_request_helper(self, name, cookie_type, void, regular, aux=False, reply_f
                 else:
                     _c('    memcpy(xcb_out.%s, %s, %d);', field.c_field_name, field.c_field_name, field.type.nmemb)
 
-    def get_serialize_args(type_obj, c_field_name, aux_var, context='serialize'):
+    def get_serialize_args(type_obj, c_field_name, aux_var, context):
         serialize_args = get_serialize_params(context, type_obj,
                                               c_field_name,
                                               aux_var)[2]
@@ -2023,22 +2013,19 @@  def _c_request_helper(self, name, cookie_type, void, regular, aux=False, reply_f
                     else:
                         # not supposed to happen
                         raise Exception("unhandled variable size field %s" % field.c_field_name)
-                else:
-                    if not aux:
-                        _c('    xcb_parts[%d].iov_base = (char *) %s;', count, field.c_field_name)
+                elif field.type.c_need_serialize:
                     idx = serial_fields.index(field)
                     aux_var = '&xcb_aux%d' % idx
-                    context = 'serialize' if aux else 'sizeof'
+                    serialize_args = get_serialize_args(field.type, aux_var, field.c_field_name, 'serialize')
                     _c('    xcb_parts[%d].iov_len =', count)
-                    if aux:
-                        serialize_args = get_serialize_args(field.type, aux_var, field.c_field_name, context)
-                        _c('      %s (%s);', field.type.c_serialize_name, serialize_args)
-                        _c('    xcb_parts[%d].iov_base = xcb_aux%d;' % (count, idx))
-                        free_calls.append('    free(xcb_aux%d);' % idx)
-                    else:
-                        serialize_args = get_serialize_args(field.type, field.c_field_name, aux_var, context)
-                        func_name = field.type.c_sizeof_name
-                        _c('      %s (%s);', func_name, serialize_args)
+                    _c('      %s (%s);', field.type.c_serialize_name, serialize_args)
+                    _c('    xcb_parts[%d].iov_base = xcb_aux%d;' % (count, idx))
+                    free_calls.append('    free(xcb_aux%d);' % idx)
+                elif field.type.c_need_sizeof:
+                    _c('    xcb_parts[%d].iov_base = (char *) %s;', count, field.c_field_name)
+                    serialize_args = get_serialize_args(field.type, field.c_field_name, '_aux', 'sizeof')
+                    _c('    xcb_parts[%d].iov_len =', count)
+                    _c('      %s (%s);', field.type.c_sizeof_name, serialize_args)
 
                 count += 1
                 if not (field.type.c_need_serialize or field.type.c_need_sizeof):
@@ -2212,10 +2199,10 @@  def _c_cookie(self, name):
     _h('    unsigned int sequence; /**<  */')
     _h('} %s;', self.c_cookie_type)
 
-def _man_request(self, name, cookie_type, void, aux):
+def _man_request(self, name, cookie_type, void):
     param_fields = [f for f in self.fields if f.visible]
 
-    func_name = self.c_request_name if not aux else self.c_aux_name
+    func_name = self.c_request_name
 
     def create_link(linkname):
         name = 'man/%s.%s' % (linkname, section)
@@ -2249,15 +2236,12 @@  def _man_request(self, name, cookie_type, void, aux):
         c_pointer = field.c_pointer
         if c_pointer == ' ':
             c_pointer = ''
-        if field.type.c_need_serialize and not aux:
-            c_field_const_type = "const void"
-            c_pointer = '*'
         comma = ', ' if count else ');'
         prototype += '%s\\ %s\\fI%s\\fP%s' % (c_field_const_type, c_pointer, field.c_field_name, comma)
 
     f.write('.SS Request function\n')
     f.write('.HP\n')
-    base_func_name = self.c_request_name if not aux else self.c_aux_name
+    base_func_name = self.c_request_name
     f.write('%s \\fB%s\\fP(xcb_connection_t\\ *\\fIconn\\fP, %s\n' % (cookie_type, base_func_name, prototype))
     create_link('%s_%s' % (base_func_name, ('checked' if void else 'unchecked')))
     if not void:
@@ -2751,11 +2735,8 @@  def c_request(self, name):
         _c_complex(self.reply)
         # Request prototypes
         has_fds = _c_reply_has_fds(self.reply)
-        _c_request_helper(self, name, self.c_cookie_type, False, True, False, has_fds)
-        _c_request_helper(self, name, self.c_cookie_type, False, False, False, has_fds)
-        if self.c_need_aux:
-            _c_request_helper(self, name, self.c_cookie_type, False, True, True, has_fds)
-            _c_request_helper(self, name, self.c_cookie_type, False, False, True, has_fds)
+        _c_request_helper(self, name, self.c_cookie_type, False, True, has_fds)
+        _c_request_helper(self, name, self.c_cookie_type, False, False, has_fds)
         # Reply accessors
         _c_accessors(self.reply, name + ('reply',), name)
         _c_reply(self, name)
@@ -2765,14 +2746,10 @@  def c_request(self, name):
         # Request prototypes
         _c_request_helper(self, name, 'xcb_void_cookie_t', True, False)
         _c_request_helper(self, name, 'xcb_void_cookie_t', True, True)
-        if self.c_need_aux:
-            _c_request_helper(self, name, 'xcb_void_cookie_t', True, False, True)
-            _c_request_helper(self, name, 'xcb_void_cookie_t', True, True, True)
 
     # We generate the manpage afterwards because _c_type_setup has been called.
-    # TODO: what about aux helpers?
     cookie_type = self.c_cookie_type if self.reply else 'xcb_void_cookie_t'
-    _man_request(self, name, cookie_type, not self.reply, False)
+    _man_request(self, name, cookie_type, not self.reply)
 
 def c_event(self, name):
     '''

Comments

On Tuesday 25 February 2014 14:11:37 Ran Benita wrote:
> Currently, for requests which take a parameter which needs to be
> serialized (like some complex struct), there are two types of functions
> generated:
>     xbc_<request_name>{,checked,unchecked}
>     xbc_<request_name>_aux{,checked,unchecked}
> The difference between the aux and non-aux versions is how they handle
> the complex argument(s):
> 
> - The aux version takes a type-safe instance of the struct, and
>   internally calls the _serialize() function for this struct, which
>   translates it to its wire representation.
> 
> - The non-aux version instead takes a void*, unpacks it to the struct in
>   order to calculate the sizeof, and then sends it directly.
> 
> The non-aux version is not type-safe and expects the user to lay out the
> data manually. People also don't know the aux version even exists, and
> thus use the non-aux version incorrectly.
> 
> Therefore, remove the non-aux versions and drop the _aux prefix
> entirely, and make the code-generator worry about one less obscurity on
> the way.

Thanks for adding me to CC.

Can I suggest to keep the _aux prefix and just delegate to the version without 
_aux? This would make life much easier as it takes about one to two years till 
we can start to depend on new stuff in the libraries. We basically have to 
wait till it is available in all the distributions which our developers use to 
build our software. And if KWin doesn't build I get yelled at ;-) That way I 
could start introducing the proper way again using _aux and don't worry about 
it going away in a future version of the library.

Cheers
Martin Gräßlin
On Tue, Feb 25, 2014 at 02:11:37PM +0200, Ran Benita wrote:
> Currently, for requests which take a parameter which needs to be
> serialized (like some complex struct), there are two types of functions
> generated:
>     xbc_<request_name>{,checked,unchecked}
>     xbc_<request_name>_aux{,checked,unchecked}
> The difference between the aux and non-aux versions is how they handle
> the complex argument(s):
> 
> - The aux version takes a type-safe instance of the struct, and
>   internally calls the _serialize() function for this struct, which
>   translates it to its wire representation.
> 
> - The non-aux version instead takes a void*, unpacks it to the struct in
>   order to calculate the sizeof, and then sends it directly.
> 
> The non-aux version is not type-safe and expects the user to lay out the
> data manually. People also don't know the aux version even exists, and
> thus use the non-aux version incorrectly.
> 
> Therefore, remove the non-aux versions and drop the _aux prefix
> entirely, and make the code-generator worry about one less obscurity on
> the way.

As much as I'd love to see this change, this would break the library
ABI.  That's not OK, and looking around for potential callers doesn't
make it OK.

- Josh Triplett
On Tue, Feb 25, 2014 at 05:36:42AM -0800, Josh Triplett wrote:
> On Tue, Feb 25, 2014 at 02:11:37PM +0200, Ran Benita wrote:
> > Currently, for requests which take a parameter which needs to be
> > serialized (like some complex struct), there are two types of functions
> > generated:
> >     xbc_<request_name>{,checked,unchecked}
> >     xbc_<request_name>_aux{,checked,unchecked}
> > The difference between the aux and non-aux versions is how they handle
> > the complex argument(s):
> > 
> > - The aux version takes a type-safe instance of the struct, and
> >   internally calls the _serialize() function for this struct, which
> >   translates it to its wire representation.
> > 
> > - The non-aux version instead takes a void*, unpacks it to the struct in
> >   order to calculate the sizeof, and then sends it directly.
> > 
> > The non-aux version is not type-safe and expects the user to lay out the
> > data manually. People also don't know the aux version even exists, and
> > thus use the non-aux version incorrectly.
> > 
> > Therefore, remove the non-aux versions and drop the _aux prefix
> > entirely, and make the code-generator worry about one less obscurity on
> > the way.
> 
> As much as I'd love to see this change, this would break the library
> ABI.  That's not OK, and looking around for potential callers doesn't
> make it OK.

OK. I don't really agree, but can't argue with being careful.

> - Josh Triplett
On Tue, Feb 25, 2014 at 01:27:18PM +0100, Martin Gräßlin wrote:
> On Tuesday 25 February 2014 14:11:37 Ran Benita wrote:
> > Currently, for requests which take a parameter which needs to be
> > serialized (like some complex struct), there are two types of functions
> > generated:
> >     xbc_<request_name>{,checked,unchecked}
> >     xbc_<request_name>_aux{,checked,unchecked}
> > The difference between the aux and non-aux versions is how they handle
> > the complex argument(s):
> > 
> > - The aux version takes a type-safe instance of the struct, and
> >   internally calls the _serialize() function for this struct, which
> >   translates it to its wire representation.
> > 
> > - The non-aux version instead takes a void*, unpacks it to the struct in
> >   order to calculate the sizeof, and then sends it directly.
> > 
> > The non-aux version is not type-safe and expects the user to lay out the
> > data manually. People also don't know the aux version even exists, and
> > thus use the non-aux version incorrectly.
> > 
> > Therefore, remove the non-aux versions and drop the _aux prefix
> > entirely, and make the code-generator worry about one less obscurity on
> > the way.
> 
> Thanks for adding me to CC.
> 
> Can I suggest to keep the _aux prefix and just delegate to the version without 
> _aux? This would make life much easier as it takes about one to two years till 
> we can start to depend on new stuff in the libraries. We basically have to 
> wait till it is available in all the distributions which our developers use to 
> build our software. And if KWin doesn't build I get yelled at ;-) That way I 
> could start introducing the proper way again using _aux and don't worry about 
> it going away in a future version of the library.

The patch is withdrawn, so I won't get into details how to make this
work. You should use the _aux version instead. It is available
(assuredly) starting from libxcb 1.10, which was released Dec 2013, so
I guess you have to wait some time yet, unless you like those yellings :)

Ran

> Cheers
> Martin Gräßlin