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

[Ran Benita]

Also another request: if/when you send a new version of the patch, it'd
be helpful to resend the matching libxcb patch with it (i.e. list both
.patch files in git send-email). That way it's easier to review in the
mailing list.

Thanks,
Ran

On Sat, Mar 28, 2015 at 11:36:59AM +0300, Ran Benita wrote:
> 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