[libxcb,1/1] Calculate length of length less lists

Submitted by Jaya Tiwari on April 23, 2015, 6:47 a.m.

Details

Message ID 1429771654-16286-1-git-send-email-tiwari.jaya18@gmail.com
State New
Headers show

Commit Message

Jaya Tiwari April 23, 2015, 6:47 a.m.
Signed-off-by: Jaya Tiwari <tiwari.jaya18@gmail.com>
---
 src/c_client.py | 54 +++++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 41 insertions(+), 13 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/c_client.py b/src/c_client.py
index e55fc3c..2e3129c 100644
--- a/src/c_client.py
+++ b/src/c_client.py
@@ -647,7 +647,7 @@  def get_expr_fields(self):
     get the Fields referenced by switch or list expression
     """
     def get_expr_field_names(expr):
-        if expr.op is None:
+        if expr.op is None or expr.op == 'calculate_len':
             if expr.lenfield_name is not None:
                 return [expr.lenfield_name]
             else:
@@ -754,6 +754,7 @@  def get_serialize_params(context, self, buffer_var='_buffer', aux_var='_aux'):
     param_fields = []
     wire_fields = []
 
+    dont_include_len = False
     for field in self.fields:
         if field.visible:
             # the field should appear as a parameter in the function call
@@ -770,6 +771,8 @@  def get_serialize_params(context, self, buffer_var='_buffer', aux_var='_aux'):
     if self.is_switch:
         param_fields = get_expr_fields(self)
 
+    if self.is_list and self.type.expr.op == 'calculate_len':
+       dont_include_len = True
     # _serialize()/_unserialize()/_unpack() function parameters
     # note: don't use set() for params, it is unsorted
     params = []
@@ -793,7 +796,7 @@  def get_serialize_params(context, self, buffer_var='_buffer', aux_var='_aux'):
             pointerspec = p.c_pointer
             add_param(params, (typespec, pointerspec, p.c_field_name))
         else:
-            if p.visible and not p.wire and not p.auto:
+            if p.visible and not p.wire and not p.auto and not dont_include_len:
                 typespec = p.c_field_type
                 pointerspec = ''
                 add_param(params, (typespec, pointerspec, p.c_field_name))
@@ -983,7 +986,10 @@  def _c_serialize_helper_list_field(context, self, field,
         if len(unresolved)>0:
             raise Exception('could not resolve the length fields required for list %s' % field.c_field_name)
 
-    list_length = _c_accessor_get_expr(expr, field_mapping)
+    if expr.op == 'calculate_len':
+        list_length = field.type.expr.lenfield_name
+    else:
+        list_length = _c_accessor_get_expr(expr, field_mapping)
 
     # default: list with fixed size elements
     length = '%s * sizeof(%s)' % (list_length, field.type.member.c_wiretype)
@@ -1719,7 +1725,7 @@  def _c_accessor_get_expr(expr, field_mapping):
         return sumvar
     elif expr.op == 'listelement-ref':
         return '(*xcb_listelement)'
-    elif expr.op != None:
+    elif expr.op != None and expr.op != 'calculate_len':
         return ('(' + _c_accessor_get_expr(expr.lhs, field_mapping) +
                 ' ' + expr.op + ' ' +
                 _c_accessor_get_expr(expr.rhs, field_mapping) + ')')
@@ -1800,6 +1806,7 @@  def _c_accessors_list(self, field):
 
     list = field.type
     c_type = self.c_type
+    dont_need_len = False
 
     # special case: switch
     # in case of switch, 2 params have to be supplied to certain accessor functions:
@@ -1900,16 +1907,25 @@  def _c_accessors_list(self, field):
     _hc('')
     _hc('int')
     spacing = ' '*(len(field.c_length_name)+2)
-    add_param_str = additional_params_to_str(spacing)
+    if field.type.is_list and field.type.expr.op == 'calculate_len':
+        dont_need_len = True
+    else:
+        add_param_str = additional_params_to_str(spacing)
     if switch_obj is not None:
         _hc('%s (const %s *R  /**< */,', field.c_length_name, R_obj.c_type)
         _h('%sconst %s *S /**< */%s);', spacing, S_obj.c_type, add_param_str)
         _c('%sconst %s *S  /**< */%s)', spacing, S_obj.c_type, add_param_str)
-    else:
+    elif not dont_need_len:
         _h('%s (const %s *R  /**< */%s);', field.c_length_name, c_type, add_param_str)
         _c('%s (const %s *R  /**< */%s)', field.c_length_name, c_type, add_param_str)
+    else:
+        _h('%s (const %s *R  /**< */);', field.c_length_name, c_type)
+        _c('%s (const %s *R  /**< */)', field.c_length_name, c_type)
     _c('{')
-    length = _c_accessor_get_expr(field.type.expr, fields)
+    if field.type.expr.op == 'calculate_len':
+        length = '(((R->length * 4) - sizeof('+ self.c_type + '))/'+'sizeof('+field.type.member.c_wiretype+'))'
+    else:
+        length = _c_accessor_get_expr(field.type.expr, fields)
     _c('    return %s;', length)
     _c('}')
 
@@ -1950,19 +1966,26 @@  def _c_accessors_list(self, field):
         _hc('')
         _hc('%s', field.c_iterator_type)
         spacing = ' '*(len(field.c_iterator_name)+2)
-        add_param_str = additional_params_to_str(spacing)
+        if field.type.expr.op != 'calculate_len':
+            add_param_str = additional_params_to_str(spacing)
         if switch_obj is not None:
             _hc('%s (const %s *R  /**< */,', field.c_iterator_name, R_obj.c_type)
             _h('%sconst %s *S /**< */%s);', spacing, S_obj.c_type, add_param_str)
             _c('%sconst %s *S  /**< */%s)', spacing, S_obj.c_type, add_param_str)
-        else:
+        elif not dont_need_len:
             _h('%s (const %s *R  /**< */%s);', field.c_iterator_name, c_type, add_param_str)
             _c('%s (const %s *R  /**< */%s)', field.c_iterator_name, c_type, add_param_str)
+        else:
+            _h('%s (const %s *R  /**< */);', field.c_iterator_name, c_type)
+            _c('%s (const %s *R  /**< */)', field.c_iterator_name, c_type)
         _c('{')
         _c('    %s i;', field.c_iterator_type)
 
         _c_pre.start()
-        length_expr_str = _c_accessor_get_expr(field.type.expr, fields)
+        if field.type.expr.op == 'calculate_len':
+            length_expr_str = '(((R->length * 4) - sizeof('+ self.c_type + '))/'+'sizeof('+field.c_field_type+'))'
+        else:
+            length_expr_str = _c_accessor_get_expr(field.type.expr, fields)
 
         if switch_obj is not None:
             _c_pre.end()
@@ -2152,6 +2175,7 @@  def _c_request_helper(self, name, void, regular, aux=False, reply_fds=False):
     serial_fields = []
     # special case: list with variable size elements
     list_with_var_size_elems = False
+    have_field_len = False
 
     for field in self.fields:
         if field.visible:
@@ -2342,9 +2366,13 @@  def _c_request_helper(self, name, void, regular, aux=False, reply_fds=False):
                     _c('    xcb_parts[%d].iov_base = (char *) %s;', count, field.c_field_name)
                     if field.type.is_list:
                         if field.type.member.fixed_size():
-                            _c('    xcb_parts[%d].iov_len = %s * sizeof(%s);', count,
-                               _c_accessor_get_expr(field.type.expr, None),
-                               field.type.member.c_wiretype)
+                            if field.type.expr.op == 'calculate_len':
+                                lenfield = field.type.expr.lenfield_name
+                            else:
+                                lenfield = _c_accessor_get_expr(field.type.expr, None)
+
+                            _c('    xcb_parts[%d].iov_len = %s * sizeof(%s);', count, lenfield,
+                                        field.type.member.c_wiretype)
                         else:
                             list_length = _c_accessor_get_expr(field.type.expr, None)
 

Comments

Ran Benita April 26, 2015, 7:34 p.m.
I haven't done a real review here - it looks like it needs a deep dive
into c_client.py which is a black hole...
That said I'll try to get to it when I have time.

On Thu, Apr 23, 2015 at 12:17:34PM +0530, Jaya Tiwari wrote:
> Signed-off-by: Jaya Tiwari <tiwari.jaya18@gmail.com>
> ---
>  src/c_client.py | 54 +++++++++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 41 insertions(+), 13 deletions(-)
> 
> diff --git a/src/c_client.py b/src/c_client.py
> index e55fc3c..2e3129c 100644
> --- a/src/c_client.py
> +++ b/src/c_client.py
> @@ -647,7 +647,7 @@ def get_expr_fields(self):
>      get the Fields referenced by switch or list expression
>      """
>      def get_expr_field_names(expr):
> -        if expr.op is None:
> +        if expr.op is None or expr.op == 'calculate_len':

Can you move the `expr.op == ...` case to below with the other `op`s?
That would make it clear which branch is supposed to be taken.

Also the name `calculate_len` sounds a bit too generic, but nothing
better comes to mind right now..

>              if expr.lenfield_name is not None:
>                  return [expr.lenfield_name]
>              else:
> @@ -754,6 +754,7 @@ def get_serialize_params(context, self, buffer_var='_buffer', aux_var='_aux'):
>      param_fields = []
>      wire_fields = []
>  
> +    dont_include_len = False

Maybe avoid the double-negation and do `include_len = True`?

>      for field in self.fields:
>          if field.visible:
>              # the field should appear as a parameter in the function call
> @@ -770,6 +771,8 @@ def get_serialize_params(context, self, buffer_var='_buffer', aux_var='_aux'):
>      if self.is_switch:
>          param_fields = get_expr_fields(self)
>  
> +    if self.is_list and self.type.expr.op == 'calculate_len':
> +       dont_include_len = True
>      # _serialize()/_unserialize()/_unpack() function parameters
>      # note: don't use set() for params, it is unsorted
>      params = []
> @@ -793,7 +796,7 @@ def get_serialize_params(context, self, buffer_var='_buffer', aux_var='_aux'):
>              pointerspec = p.c_pointer
>              add_param(params, (typespec, pointerspec, p.c_field_name))
>          else:
> -            if p.visible and not p.wire and not p.auto:
> +            if p.visible and not p.wire and not p.auto and not dont_include_len:
>                  typespec = p.c_field_type
>                  pointerspec = ''
>                  add_param(params, (typespec, pointerspec, p.c_field_name))
> @@ -983,7 +986,10 @@ def _c_serialize_helper_list_field(context, self, field,
>          if len(unresolved)>0:
>              raise Exception('could not resolve the length fields required for list %s' % field.c_field_name)
>  
> -    list_length = _c_accessor_get_expr(expr, field_mapping)
> +    if expr.op == 'calculate_len':
> +        list_length = field.type.expr.lenfield_name
> +    else:
> +        list_length = _c_accessor_get_expr(expr, field_mapping)
>  
>      # default: list with fixed size elements
>      length = '%s * sizeof(%s)' % (list_length, field.type.member.c_wiretype)
> @@ -1719,7 +1725,7 @@ def _c_accessor_get_expr(expr, field_mapping):
>          return sumvar
>      elif expr.op == 'listelement-ref':
>          return '(*xcb_listelement)'
> -    elif expr.op != None:
> +    elif expr.op != None and expr.op != 'calculate_len':

As long as you're touching this line you might as well fix the `!= None`
to be `is not None` which is what python prefers.

Ran

>          return ('(' + _c_accessor_get_expr(expr.lhs, field_mapping) +
>                  ' ' + expr.op + ' ' +
>                  _c_accessor_get_expr(expr.rhs, field_mapping) + ')')
> @@ -1800,6 +1806,7 @@ def _c_accessors_list(self, field):
>  
>      list = field.type
>      c_type = self.c_type
> +    dont_need_len = False
>  
>      # special case: switch
>      # in case of switch, 2 params have to be supplied to certain accessor functions:
> @@ -1900,16 +1907,25 @@ def _c_accessors_list(self, field):
>      _hc('')
>      _hc('int')
>      spacing = ' '*(len(field.c_length_name)+2)
> -    add_param_str = additional_params_to_str(spacing)
> +    if field.type.is_list and field.type.expr.op == 'calculate_len':
> +        dont_need_len = True
> +    else:
> +        add_param_str = additional_params_to_str(spacing)
>      if switch_obj is not None:
>          _hc('%s (const %s *R  /**< */,', field.c_length_name, R_obj.c_type)
>          _h('%sconst %s *S /**< */%s);', spacing, S_obj.c_type, add_param_str)
>          _c('%sconst %s *S  /**< */%s)', spacing, S_obj.c_type, add_param_str)
> -    else:
> +    elif not dont_need_len:
>          _h('%s (const %s *R  /**< */%s);', field.c_length_name, c_type, add_param_str)
>          _c('%s (const %s *R  /**< */%s)', field.c_length_name, c_type, add_param_str)
> +    else:
> +        _h('%s (const %s *R  /**< */);', field.c_length_name, c_type)
> +        _c('%s (const %s *R  /**< */)', field.c_length_name, c_type)
>      _c('{')
> -    length = _c_accessor_get_expr(field.type.expr, fields)
> +    if field.type.expr.op == 'calculate_len':
> +        length = '(((R->length * 4) - sizeof('+ self.c_type + '))/'+'sizeof('+field.type.member.c_wiretype+'))'
> +    else:
> +        length = _c_accessor_get_expr(field.type.expr, fields)
>      _c('    return %s;', length)
>      _c('}')
>  
> @@ -1950,19 +1966,26 @@ def _c_accessors_list(self, field):
>          _hc('')
>          _hc('%s', field.c_iterator_type)
>          spacing = ' '*(len(field.c_iterator_name)+2)
> -        add_param_str = additional_params_to_str(spacing)
> +        if field.type.expr.op != 'calculate_len':
> +            add_param_str = additional_params_to_str(spacing)
>          if switch_obj is not None:
>              _hc('%s (const %s *R  /**< */,', field.c_iterator_name, R_obj.c_type)
>              _h('%sconst %s *S /**< */%s);', spacing, S_obj.c_type, add_param_str)
>              _c('%sconst %s *S  /**< */%s)', spacing, S_obj.c_type, add_param_str)
> -        else:
> +        elif not dont_need_len:
>              _h('%s (const %s *R  /**< */%s);', field.c_iterator_name, c_type, add_param_str)
>              _c('%s (const %s *R  /**< */%s)', field.c_iterator_name, c_type, add_param_str)
> +        else:
> +            _h('%s (const %s *R  /**< */);', field.c_iterator_name, c_type)
> +            _c('%s (const %s *R  /**< */)', field.c_iterator_name, c_type)
>          _c('{')
>          _c('    %s i;', field.c_iterator_type)
>  
>          _c_pre.start()
> -        length_expr_str = _c_accessor_get_expr(field.type.expr, fields)
> +        if field.type.expr.op == 'calculate_len':
> +            length_expr_str = '(((R->length * 4) - sizeof('+ self.c_type + '))/'+'sizeof('+field.c_field_type+'))'
> +        else:
> +            length_expr_str = _c_accessor_get_expr(field.type.expr, fields)
>  
>          if switch_obj is not None:
>              _c_pre.end()
> @@ -2152,6 +2175,7 @@ def _c_request_helper(self, name, void, regular, aux=False, reply_fds=False):
>      serial_fields = []
>      # special case: list with variable size elements
>      list_with_var_size_elems = False
> +    have_field_len = False
>  
>      for field in self.fields:
>          if field.visible:
> @@ -2342,9 +2366,13 @@ def _c_request_helper(self, name, void, regular, aux=False, reply_fds=False):
>                      _c('    xcb_parts[%d].iov_base = (char *) %s;', count, field.c_field_name)
>                      if field.type.is_list:
>                          if field.type.member.fixed_size():
> -                            _c('    xcb_parts[%d].iov_len = %s * sizeof(%s);', count,
> -                               _c_accessor_get_expr(field.type.expr, None),
> -                               field.type.member.c_wiretype)
> +                            if field.type.expr.op == 'calculate_len':
> +                                lenfield = field.type.expr.lenfield_name
> +                            else:
> +                                lenfield = _c_accessor_get_expr(field.type.expr, None)
> +
> +                            _c('    xcb_parts[%d].iov_len = %s * sizeof(%s);', count, lenfield,
> +                                        field.type.member.c_wiretype)
>                          else:
>                              list_length = _c_accessor_get_expr(field.type.expr, None)
>  
> -- 
> 1.9.1
> 
> _______________________________________________
> Xcb mailing list
> Xcb@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/xcb