[Xcb] [PATCH proto 1/1 RESEND] Calculate length of lengthless lists

[Ran Benita]

On Sat, Mar 21, 2015 at 12:09:49AM +0530, Jaya Tiwari wrote:
> Added handling of length less list elements which are the only
> variable part of the request.
> Such lists that are the only variable part with all other fixed sized
> elements declared before it are identified here.

Can you give an example of a request or reply which has a lengthless
list? Also, what in the problem with the current handling?

I'm not sure what's the context of the patch, so for now I'll only
comment on the code itself.

> Signed-off-by: Jaya Tiwari <tiwari.jaya18 at gmail.com>
> ---
>  xcbgen/xtypes.py | 21 ++++++++++++++++++---
>  1 file changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/xcbgen/xtypes.py b/xcbgen/xtypes.py
> index 4d6bbc0..1e2ad55 100644
> --- a/xcbgen/xtypes.py
> +++ b/xcbgen/xtypes.py
> @@ -176,16 +176,27 @@ class ListType(Type):
>      parent is the structure type containing the list.
>      expr is an Expression object containing the length information, for variable-sized lists.
>      '''
> -    def __init__(self, elt, member, *parent):
> +    def __init__(self, elt, member, no_of_var_fields, *parent):

"num" is a more common acronym for "number".

>          Type.__init__(self, member.name)
>          self.is_list = True
>          self.member = member
>          self.parents = list(parent)
> +        lenfield_name = False

`lenfield_name` sounds like a string, why is it initialized with `False`
(a boolean)?

> +        has_request = False
> +        has_lenfield_ref = False
> +        dont_have_len = False

Maybe `have_len = True` instead of double negation?

Actually, I don't see `dont_have_len`, `has_lenfield_ref` and
`lenfield_name` used anywhere in this function, why add them?

>  
>          if elt.tag == 'list':
>              elts = list(elt)
> -            self.expr = Expression(elts[0] if len(elts) else elt, self)
>  
> +            if 'Request' in str(self.parents):

This test is not very robust, please use a proper test here instead of
relying on name pretty-printing.

> +                has_request = True

`has_request` is only used inside this if-branch right? If so, you can
move the initialization here and write

    has_request = 'Request' in str(self.parents)

(but with the proper test of course).

> +            if not len(elts) and has_request and no_of_var_fields == 1:

Please change `not len(elts)` to `len(elts) == 0`; `not` should be
reserved for boolean IMO.

> +                self.expr = Expression(elt,self)
> +                self.expr.op = 'calculate_len'
> +            else:
> +                self.expr = Expression(elts[0] if len(elts) else elt, self)
> +        
>          self.size = member.size if member.fixed_size() else None
>          self.nmemb = self.expr.nmemb if self.expr.fixed_size() else None
>  
> @@ -302,7 +313,11 @@ class ComplexType(Type):

Please change tabs in this hunk to spaces. I haven't tried it, but
python3 rejects mixed tabs and spaces (TabError):
https://www.python.org/dev/peps/pep-0008/#tabs-or-spaces
python3 works fine so it'd be a shame to break it.

>          if self.resolved:
>              return
>          enum = None
> +	no_of_var_fields = 0
>  
> +	for child in list(self.elt):

Is `list()` necessary here?

> +	    if child.tag != 'pad' and child.tag != 'field':
> +		no_of_var_fields = no_of_var_fields + 1
>          # Resolve all of our field datatypes.
>          for child in list(self.elt):
>              if child.tag == 'pad':
> @@ -325,7 +340,7 @@ class ComplexType(Type):
>              elif child.tag == 'list':
>                  field_name = child.get('name')
>                  fkey = child.get('type')
> -                type = ListType(child, module.get_type(fkey), *self.lenfield_parent)
> +                type = ListType(child, module.get_type(fkey), no_of_var_fields, *self.lenfield_parent)
>                  visible = True
>              elif child.tag == 'switch':
>                  field_name = child.get('name')
> -- 
> 1.8.1.2
> 
> _______________________________________________
> Xcb mailing list
> Xcb at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/xcb