[Xcb] [PATCH libxcb 2/3] generator: support lists of structs which contain a switch

[Ran Benita]

Mentioning a request which needs this in the commit message would be
nice!

On Thu, Aug 21, 2014 at 10:35:55PM +0200, Christian Linhart wrote:
> This essentially requires to have a correct sizeof-function
> for the struct.
> This in turn requires a sizeof-function for the switch, too.
> 
> Making a sizeof-function for the switch is triggered by
> replacing "elif" by "if" in the first change of this patch.
> This way, c_need_sizeof is also set to True for switches if appropriate.
> 
> The _c_serialize_helper_switch_field function has to support
> the context "sizeof":
> This is done in the second change of this patch
> 
> The third change of this patch fixes an alignment error:
> It does not make sense to base the padding on the struct-type
> which is generated for switch because this struct does not
> represent the protocol. Rather it is the output of deserialization.
> ( The implicit padding for var-sized fields has other issues, IMHO,
> but I am not touching these now...)
> 
> The effect on the generated code for the current xml-files
> is as follows:
> * several additional sizeof-functions are generated
> * the fix of the alignment error only changes one place
>   in the XKB-extension for the GetKbdByName-reply.
>   This is no problem because that reply in its current form
>   is broken/unfinished anyways.
> 
> Note:
> This patch also fixes a problem in the generator when
> a fixed-size list is the last field of a case or bitcase.
> 
> ---
>  src/c_client.py | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/src/c_client.py b/src/c_client.py
> index fa59fff..1ed486c 100644
> --- a/src/c_client.py
> +++ b/src/c_client.py
> @@ -354,15 +354,16 @@ def _c_type_setup(self, name, postfix):
>              if field.type.is_list and not field.type.member.fixed_size():
>                  field.c_pointer = '*'
>  
>              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_case_or_bitcase:
> +
> +            if not field.type.fixed_size() and not field.type.is_case_or_bitcase:
>                  self.c_need_sizeof = True

The switches are internal, they can not be used directly e.g. by another
extension module, right? I wish we could generate them only when needed,
or make them static so the compiler can prune them. Right now most
sizeof()'s are just dead .text. But this one doesn't add too many, so OK.

Second, how hard would it be to make the above condition positive? (i.e.
this or that or that).

>  
>              field.c_iterator_type = _t(field.field_type + ('iterator',))      # xcb_fieldtype_iterator_t
>              field.c_iterator_name = _n(name + (field.field_name, 'iterator')) # xcb_container_field_iterator
>              field.c_accessor_name = _n(name + (field.field_name,))            # xcb_container_field
>              field.c_length_name = _n(name + (field.field_name, 'length'))     # xcb_container_field_length
>              field.c_end_name = _n(name + (field.field_name, 'end'))           # xcb_container_field_end
> @@ -766,14 +767,18 @@ def _c_serialize_helper_switch_field(context, self, field, c_switch_variable, pr
>      # call _serialize()/_unpack() to determine the actual size
>      if 'serialize' == context:
>          length = "%s(&%s, %s&%s%s)" % (field.type.c_serialize_name, c_switch_variable,
>                                         c_field_names, prefix_str, field.c_field_name)
>      elif context in ('unserialize', 'unpack'):
>          length = "%s(xcb_tmp, %s&%s%s)" % (field.type.c_unpack_name,
>                                             c_field_names, prefix_str, field.c_field_name)
> +    elif 'sizeof' == context:
> +        #remove trailing ", " from c_field_names because it will be used at end of arglist
> +        my_c_field_names = c_field_names[:-2]
> +        length = "%s( xcb_tmp, %s )" % ( field.type.c_sizeof_name, my_c_field_names )
>  
>      return length
>  # _c_serialize_helper_switch_field()
>  
>  def _c_serialize_helper_list_field(context, self, field,
>                                     code_lines, temp_vars,
>                                     space, prefix):
> @@ -1042,15 +1047,20 @@ def _c_serialize_helper_fields(context, self,
>  
>          if 'serialize' == context:
>              if '' != length:
>                  code_lines.append('%s    xcb_parts[xcb_parts_idx].iov_len = %s;' % (space, length))
>              code_lines.append('%s    xcb_parts_idx++;' % space)
>              count += 1
>  
> -        code_lines.append('%s    xcb_align_to = ALIGNOF(%s);' % (space, 'char' if field.c_field_type == 'void' else field.c_field_type))
> +        code_lines.append(
> +            '%s    xcb_align_to = ALIGNOF(%s);'
> +             % (space,
> +                 'char'
> +                  if field.c_field_type == 'void' or field.type.is_switch
> +                  else field.c_field_type))

Right, using the type doesn't make sense. Would be better as a separate
commit though! (Also, maybe extract the if-else to a variable?)

Anyway, looks good to me:

Reviewed-by: Ran Benita <ran234 at gmail.com>

>  
>          need_padding = True
>          if self.c_var_followed_by_fixed_fields:
>              need_padding = False
>  
>      return count
>  # _c_serialize_helper_fields()
> -- 
> 2.0.1
> 
> _______________________________________________
> Xcb mailing list
> Xcb at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/xcb