[Xcb] [PATCH libxcb 1/6] generator: fix absname for fields with only accessor function

[Ran Benita]

On Wed, Sep 03, 2014 at 01:22:36PM +0200, Christian Linhart wrote:
> Fix _c_helper_absolute_name for fields which cannot be accessed
> as a struct/union member but which can be accessed by an
> accessor function.
> 
> The fix calls the accessor function in these cases.
> 
> Example:
> <struct name="AbsnameTest">
>    <field type="CARD32" name="len" />
>    <list type="CARD8" name="mylist1">
>        <fieldref>len</fieldref>
>    </list>
>    <list type="CARD8" name="mylist2">
>        <sumof ref="mylist1"/>
>    </list>
> </struct>
> 
> The sumof-expression ( <sumof ref="mylist1"/> ) refers to mylist1
> which is only acessible by an accessor function.
> 
> Previously, sumof was only used inside bitcases,
> where such lists are accessible by members of the
> deserialized parent struct.
> (there is a difference between deserialization of switches
> and structs.)
> ---
>  src/c_client.py | 28 +++++++++++++++++++++++++++-
>  1 file changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/src/c_client.py b/src/c_client.py
> index 9216c7f..488f0e6 100644
> --- a/src/c_client.py
> +++ b/src/c_client.py
> @@ -430,24 +430,50 @@ def _c_type_setup(self, name, postfix):
>  def _c_helper_absolute_name(prefix, field=None):

This function is strange; most of the time it generates complete garbage
(e.g. `foo->...->`) which is subsequently ignored. So it is called for
no reason many times.

>      """
>      turn prefix, which is a list of tuples (name, separator, Type obj) into a string
>      representing a valid name in C (based on the context)
>      if field is not None, append the field name as well
>      """
>      prefix_str = ''
> +    last_sep =''
>      for name, sep, obj in prefix:
> +        if '' != last_sep:
> +            prefix_str += last_sep
>          prefix_str += name
>          if '' == sep:

(This can never happen. I have a patch to remove this).

>              sep = '->'
>              if ((obj.is_case_or_bitcase and obj.has_name) or     # named bitcase
>                  (obj.is_switch and len(obj.parents)>1)):
>                  sep = '.'
> -        prefix_str += sep
> +        last_sep = sep
> +
> +    prefix_str_without_lastsep = prefix_str
> +    prefix_str += last_sep
> +
>      if field is not None:
>          prefix_str += _cpp(field.field_name)
> +
> +
> +    if (

This is a formidable list of conditions. Are all of them needed? Can
this be extracted to a function `field_needs_accessor` or something like
that?

> +        field != None

With `None` it is customary to write `field is None`, `field is not
None`.

> +        and hasattr(field, 'c_accessor_name')
> +        and field.parent != None
> +        and field.parent.is_container
> +        and not field.parent.is_switch
> +        and not field.parent.is_case_or_bitcase
> +        and ( #the following conditions are taken from _c_accessors()
> +                ( field.type.is_list and not field.type.fixed_size() )
> +                or
> +                ( field.prev_varsized_field is not None
> +                  or not field.type.fixed_size()

This says `or` but the parens make it look like it should be `and`?

> +                )
> +        )
> +    ):
> +        prefix_str = field.c_accessor_name + "(" + prefix_str_without_lastsep + ")";

I'm not entirely convinced that this should be in this function - or the
function should be renamed. Is the above the correct thing to do in all
of the callsites?

Sorry for having more questions than answers; it is not for lack of
trying..

Ran

> +
>      return prefix_str
>  # _c_absolute_name
>  
>  def _c_helper_field_mapping(complex_type, prefix, flat=False):
>      """
>      generate absolute names, based on prefix, for all fields starting from complex_type
>      if flat == True, nested complex types are not taken into account
> -- 
> 2.0.1
> 
> _______________________________________________
> Xcb mailing list
> Xcb at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/xcb