[Xcb] [PATCH proto 1/1] Calculate length of length less lists

[Ran Benita]

On Thu, Apr 23, 2015 at 12:17:04PM +0530, Jaya Tiwari wrote:

Please include something like the explanation you sent earlier in the
commit messages (both for the proto and libxcb since they're different
repos. You can copy/paste).

Here's a nice article about why that's helpful:
http://who-t.blogspot.co.il/2009/12/on-commit-messages.html

> Signed-off-by: Jaya Tiwari <tiwari.jaya18 at gmail.com>

Overall I like the approach of giving this an explicit (in the object,
not the XML) `op` value - it simplifies all users in exchange for some
localized logic in xcbgen (unless the logic turns out to be very
simple).

> ---
>  xcbgen/xtypes.py | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/xcbgen/xtypes.py b/xcbgen/xtypes.py
> index 4d6bbc0..7e69493 100644
> --- a/xcbgen/xtypes.py
> +++ b/xcbgen/xtypes.py
> @@ -176,15 +176,23 @@ 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, num_of_var_fields, *parent):
>          Type.__init__(self, member.name)
>          self.is_list = True
>          self.member = member
>          self.parents = list(parent)
> +        has_request = False
>  
>          if elt.tag == 'list':
>              elts = list(elt)
> -            self.expr = Expression(elts[0] if len(elts) else elt, self)

The Expression __init__ says:

    if elt.tag == 'list':
        # List going into a request, which has no length field (inferred by server)
        self.lenfield_name = elt.get('name') + '_len'
        self.lenfield_type = 'CARD32'

Is there a case where the extra checks `has_request` and
`num_of_var_fields == 1` do not hold when we get inside here?
If not, we can just assert it and then this logic is not required.
If yes, then won't these cases be problematic for server-side XCB also
with this patch?

> +
> +            if 'request' in str(self.parents[0].elt.tag):

This should be better:

    has_request = self.parents[0].elt.tag == 'request'

And you can also remove the initialization `has_request = False` above.

The name `has_request` is not so clear though - it means to say whether
this list is *in* a request; so maybe `is_in_request` or even
`is_list_in_request`?

> +                has_request = True
> +            if len(elts) == 0 and has_request and num_of_var_fields == 1:
> +                self.expr = Expression(elt,self)

Please put a space after the comma, i.e. `elt, self`.

> +                self.expr.op = 'calculate_len'

The logic is neatly organized with comments in Expression.__init__ right
now, I think we should keep it all there instead splitting it across
various places.

> +            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 +310,11 @@ class ComplexType(Type):
>          if self.resolved:
>              return
>          enum = None
> +       num_of_var_fields = 0

I don't really understand this test. The case we are dealing with is a
list which appears at the end of a request and whose length is implied
by the total request length. But I don't see a reason why such a list
cannot be preceded other variable-length fields whose length is not
implicit.

So (like above) if this test always holds we should just assert it;
otherwise the exceptions to the rule aren't handled correctly here.

>  
> +       for child in self.elt:
> +           if child.tag != 'pad' and child.tag != 'field':

Not sure if this is a good test for varible-length but I'll leave it for
the next round (if it survives).

Ran

> +               num_of_var_fields = num_of_var_fields + 1
>          # Resolve all of our field datatypes.
>          for child in list(self.elt):
>              if child.tag == 'pad':
> @@ -325,7 +337,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), num_of_var_fields, *self.lenfield_parent)
>                  visible = True
>              elif child.tag == 'switch':
>                  field_name = child.get('name')
> -- 
> 1.9.1
> 
> _______________________________________________
> Xcb mailing list
> Xcb at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/xcb