[Xcb] [PATCH proto 1/1 RESEND] Calculate length of lengthless lists
Jaya Tiwari
tiwari.jaya18 at gmail.com
Wed Apr 1 11:01:00 PDT 2015
Hi Ran,
Thankyou for the comments and sorry for not including the context here.
Have a brief context included below now.
In current XCB, the length of the rectangles list of request
SetPictureClipRectangles
is not specified in render.xml:
<request name="
SetPictureClipRectangles" opcode="6">
<field type="PICTURE" name="picture" />
<field type="INT16" name="clip_x_origin" />
<field type="INT16" name="clip_y_origin" />
<list type="RECTANGLE" name="rectangles" />
</request>
This needs to be fixed for some applications, including server-side XCB.
So, the xml will be changed as follows, by using the length-field of
the request to compute the length of the rectangles-list.
So here, such length less lists are identified and are then generator
generates a suitable length expressions for such identified lists from
previous passes.
1 . I have changed the following things suggested by you :
> + def __init__(self, elt, member, no_of_var_fields, *parent):
"num" is a more common acronym for "number".
Changed to num.
2. Actually, I don't see `dont_have_len`, `has_lenfield_ref` and
`lenfield_name` used anywhere in this function, why add them?
Yes, I had added them previously for more test cases, but later
discovered they are no longer needed. Sorry for not removing them
earlier, removed now.
3. 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.
Actually I have to just make sure that the lengthless lists have
length calculated for requests only, and no other container.
4. > + 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.
Yes, Thankyou for pointing it out. Changed this one too.
Can you please suggest some solution for request checking too, is
there any way I can check the tag of parent in ListType, or check and
pass it from resolve of ComplexType ?
Meanwhile, I am trying to figure out a workaround too, and when done,
will post a patch with your mentioned corrections and will take care
while resending the patch.
Thankyou for helping out.
Regards,
Jaya
On Sat, Mar 28, 2015 at 2:14 PM, Ran Benita <ran234 at gmail.com> wrote:
> 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
--
Regards,
Jaya
More information about the Xcb
mailing list