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

[Christian Linhart]

On 10/12/14 18:33, Ran Benita wrote:
> Mentioning a request which needs this in the commit message would be
> nice!
The whole patchset is for getting the reply of request QueryDeviceState to work.
So it is at least needed there. (and probably needed for more requests which I completed in the xml later. )

I guess I should mention such an example-request for each individual patch
and not just in the start-message of a patch-thread,
so that all of the info is in one place for reviewers.

> 
> 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? 
No necessarily.
The sizeof function is not generated for the switch itself, 
but it is generated for a struct which contains the switch.

Such a struct may be used in another extension module.
Then that other extension module may need to access the sizeof function.

( So, optimizing that would require some global cross-module optimization logic in the generator. 
We probably want to avoid that complexity. :-) )

> 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.
Good that the impact is within reason.
 
> Second, how hard would it be to make the above condition positive? (i.e.
> this or that or that).
I did not change the condition from the original code in order not to break anything.
Of course, one "not" can be removed by applying boolean algebra:

   "if not field.type.fixed_size() and not field.type.is_case_or_bitcase:"
   is equivalent to
   "if not (field.type.fixed_size() or field.type.is_case_or_bitcase):"

Would that be preferable?

Another option would be to define
* a function named "variable_size" to the Type class in xcb/proto/xcbgen/xtypes.py:

	def variable_size(self):
	    return not self.fixed_size()

* a function which somehow is a positive test for something which is not a case or bitcase
  (and possibly excludes something more)

but I am not sure whether that improves code readability because the reader needs to know
how these new functions relate to their complementary counterparts...

Anyways I digress now, I guess. :-)

> 
>>  
>>              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! 
Thank you for that feedback. I'll try to separate my future changes in even smaller pieces.

> (Also, maybe extract the if-else to a variable?)
I'll put that on a TODO.list for a cleanup task.

> 
> Anyway, looks good to me:
> 
> Reviewed-by: Ran Benita <ran234 at gmail.com>

Thank you for reviewing it!

Chris

> 
>>  
>>          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
>