[Xcb] [PATCH libxcb 1/6 V3] generator: fix absname for fields with only accessor function
Ran Benita
ran234 at gmail.com
Sat Nov 1 03:06:45 PDT 2014
On Thu, Oct 30, 2014 at 01:56:33PM +0100, 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 generates calls to 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.)
>
> V2 of this patch:
> * replaced "!= None" with "is not None" because that's more pythonic.
> (according to suggestion from Ran Benita)
>
> V3 of this patch: simplification:
> * fixed the recursion in _c_type_setup
> so that _c_helper_absolute_name does not need check
> a gazillion things as a workaround anymore.
>
> * simplified _c_helper_absolute_name
> - remove unneeded check for empty string before
> append of last_sep to prefix_str
>
> - removed those if-conditions which are not
> needed anymore after fixing the recursion
> in _c_type_setup.
>
> - extract functionality for checking whether a field
> needs an accessor ( and which type of accessor )
> in functions.
> (also extracted from _c_accessors)
>
> - rearrange the condition branches and actions for
> more readability.
>
> V3 generates exactly the same *.c and *.h files as V2.
Great! This version is much nicer, this for figuring this out. I have
some immaterial nitpicky suggestions below, but in any case:
Reviewed-by: Ran Benita <ran234 at gmail.com>
> ---
> src/c_client.py | 78 ++++++++++++++++++++++++++++++++++++++++++++++++---------
> 1 file changed, 66 insertions(+), 12 deletions(-)
>
> diff --git a/src/c_client.py b/src/c_client.py
> index 56a1766..4b35ec8 100644
> --- a/src/c_client.py
> +++ b/src/c_client.py
> @@ -332,20 +332,14 @@ def _c_type_setup(self, name, postfix):
>
> self.c_container = 'union' if self.is_union else 'struct'
> prev_varsized_field = None
> prev_varsized_offset = 0
> first_field_after_varsized = None
>
> for field in self.fields:
> - _c_type_setup(field.type, field.field_type, ())
> - if field.type.is_list:
> - _c_type_setup(field.type.member, field.field_type, ())
> - if (field.type.nmemb is None):
> - self.c_need_sizeof = True
> -
> field.c_field_type = _t(field.field_type)
> field.c_field_const_type = ('' if field.type.nmemb == 1 else 'const ') + field.c_field_type
> field.c_field_name = _cpp(field.field_name)
> field.c_subscript = '[%d]' % field.type.nmemb if (field.type.nmemb and field.type.nmemb > 1) else ''
> field.c_pointer = ' ' if field.type.nmemb == 1 else '*'
>
> # correct the c_pointer field for variable size non-list types
> @@ -387,14 +381,25 @@ def _c_type_setup(self, name, postfix):
> prev_varsized_field = field
> prev_varsized_offset = 0
>
> if self.c_var_followed_by_fixed_fields:
> if field.type.fixed_size():
> field.prev_varsized_field = None
>
> + # recurse into this field
> + # this has to be done here, i.e., after the field has been set up
> + # Otherwise the function _c_helper_absolute_name
> + # will produce garbage or crash
The linebreaks are a bit unusual, maybe like this?
# Recurse into this field. This has to be done here, i.e.,
# after the field has been set up, otherwise the function
# _c_helper_absolute_name() will produce garbage or crash.
> + _c_type_setup(field.type, field.field_type, ())
> + if field.type.is_list:
> + _c_type_setup(field.type.member, field.field_type, ())
> + if (field.type.nmemb is None):
> + self.c_need_sizeof = True
> +
> +
Unneeded newline.
> if self.c_need_serialize:
> # when _unserialize() is wanted, create _sizeof() as well for consistency reasons
> self.c_need_sizeof = True
>
> # as switch does never appear at toplevel,
> # continue here with type construction
> if self.is_switch:
> @@ -423,31 +428,80 @@ def _c_type_setup(self, name, postfix):
> if self.c_need_sizeof:
> if self.c_sizeof_name not in finished_sizeof:
> if not module.namespace.is_ext or self.name[:2] == module.namespace.prefix:
> finished_sizeof.append(self.c_sizeof_name)
> _c_serialize('sizeof', self)
> # _c_type_setup()
>
> +# Functions for querying field properties
> +def _c_field_needs_list_accessor(field):
> + return field.type.is_list and not field.type.fixed_size()
> +
> +def _c_field_needs_field_accessor(field):
> + if field.type.is_list:
> + return False
> + else:
> + return (
> + field.prev_varsized_field is not None
> + or not field.type.fixed_size()
> + )
Maybe like this?
if field.type.is_list:
return False
return (field.prev_varsized_field is not None or
not field.type.fixed_size())
Or like this
return not field.type.is_list and (field.prev_varsized_field is not None or
not field.type.fixed_size())
> +
> +def _c_field_needs_accessor(field):
> + return (
> + _c_field_needs_list_accessor(field)
> + or
> + _c_field_needs_field_accessor(field)
> + )
Maybe like this? (Also some tabs here).
return (_c_field_needs_list_accessor(field) or
_c_field_needs_field_accessor(field))
> +
> +def _c_field_is_member_of_case_or_bitcase(field):
> + if field.parent is None:
> + return False
> + elif field.parent.is_case_or_bitcase:
> + return True
> + else:
> + return False
Maybe like this? (Also some tabs here).
return field.parent and field.parent.is_case_or_bitcase
Also, I don't recall from your xcbgen patch, but if `parent` is always
set for fields, it'd be better to omit the None test.
> +
> +# end of Functions for querying field properties
I don't think this comment is needed!
> +
> def _c_helper_absolute_name(prefix, field=None):
> """
> 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:
> - prefix_str += name
> + prefix_str += last_sep + name
> if '' == sep:
> 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
> - if field is not None:
> - prefix_str += _cpp(field.field_name)
> + last_sep = sep
> +
> + if field is None:
> + # add separator for access to a yet unknown field
> + prefix_str += last_sep
> + else:
> + if _c_field_needs_accessor(field):
> + if _c_field_is_member_of_case_or_bitcase(field):
So, just to check if I get it right: case members *do* need accessors
generated for them in general. So we cannot just change
`_c_field_needs_accessor()` to return false for them, as that would
mean the accessors won't be generated at all (per hunk below this one).
But using the accessor here is a headache and not strictly needed, so
we access as struct member. OK.
Question: do you know the original reason for generating accessors for
case members?
> + # case members are available in the deserialized struct,
> + # so there is no need to use the accessor function
> + # (also, their accessor function needs a different arglist
> + # so this would require special treatment here)
> + # Therefore: Access as struct member
> + prefix_str += last_sep + _cpp(field.field_name)
Maybe fold the `case_or_bitcase` condition into the one above it, such
that it falls to the `else` and this line isn't repeated?
> + else:
> + # Access with the accessor function
> + prefix_str = field.c_accessor_name + "(" + prefix_str + ")"
> + else:
> + # Access as struct member
> + prefix_str += last_sep + _cpp(field.field_name)
> +
> 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
> @@ -1730,17 +1784,17 @@ def _c_accessors(self, name, base):
> # switch always needs to be unpacked explicitly
> # if self.is_switch:
> # pass
> # else:
> if True:
Maybe we can clean this stuff above sometime.
> for field in self.fields:
> if not field.type.is_pad:
> - if field.type.is_list and not field.type.fixed_size():
> + if _c_field_needs_list_accessor(field):
> _c_accessors_list(self, field)
> - elif field.prev_varsized_field is not None or not field.type.fixed_size():
> + elif _c_field_needs_field_accessor(field):
> _c_accessors_field(self, field)
>
> def c_simple(self, name):
> '''
> Exported function that handles cardinal type declarations.
> These are types which are typedef'd to one of the CARDx's, char, float, etc.
> '''
> --
> 2.0.1
>
More information about the Xcb
mailing list