[Xcb] [PATCH lib] c_client.py: Do not create pointers in unions

[Daniel Martin]

On Fri, Dec 28, 2012 at 05:47:53PM -0800, Josh Triplett wrote:
> On Sat, Dec 29, 2012 at 01:32:09AM +0100, Daniel Martin wrote:
> > On Fri, Dec 28, 2012 at 04:18:57PM -0800, Josh Triplett wrote:
> > > On Fri, Dec 28, 2012 at 11:40:29PM +0100, Daniel Martin wrote:
> > > > Do not create pointers in unions for fields of variadic length.
> > > > 
> > > > Signed-off-by: Daniel Martin <consume.noise at gmail.com>
> > > > ---
> > > > 
> > > > With this patch xcb_xkb_doodad_t in xkb.h will change:
> > > > 
> > > >  typedef union xcb_xkb_doodad_t {
> > > >      xcb_xkb_common_doodad_t    common; /**<  */
> > > >      xcb_xkb_shape_doodad_t     shape; /**<  */
> > > > -    xcb_xkb_text_doodad_t     *text; /**<  */
> > > > +    xcb_xkb_text_doodad_t      text; /**<  */
> > > >      xcb_xkb_indicator_doodad_t indicator; /**<  */
> > > > -    xcb_xkb_logo_doodad_t     *logo; /**<  */
> > > > +    xcb_xkb_logo_doodad_t      logo; /**<  */
> > > >  } xcb_xkb_doodad_t;
> > > 
> > > Would this change any non-xkb structure in the existing ABI?
> > 
> > No, that's the only diff that came up. Every other union has
> > fixed-size-only fields.
> 
> And does this propagate the "variadic length" property to the union?

Those pointers have been there to mark a field that it has a variadic
length? Then this patch removes that information and doesn't propagate
it in another way.


I did that change while working on xinput and there it makes "list of
unions with fields of variadic size" work. But, those fields are much
more easy to handle, an abstract example follows.


Back to xcb_xkb_doodad_t. The size couldn't be guessed before and after
this patch. The function xcb_xkb_doodad_next() still contains:
    /* FIXME - determine the size of the union xcb_xkb_doodad_t */
The size can't be guessed, cause most fields (all?) have a different
size. To know the size of the union, we would need to know which field
we have to compute. Therefor, something like a switch for sizeof needs
to implemented.

I think, haven't proved it, that the requests GetGeometry and
SetGeometry in xkb are broken. At least when it comes to handle the list
of xcb_xkb_doodad_t. So, with this patch it doesn't get much worse.
Just, the "information" that a field has a variadic length gets lost.


Now, something more encouraging - list of unions in xinput, even with
fields of variadic length:
In XI1 and XI2 there are a lot of lists with structs that can be broken
down to unions. An abstract example:

    struct FooClass {
        uint8_t type;
        uint8_t len;
        uint8_t num;
        /* here comes a list with num elements */
    }
    struct BarClass {
        uint8_t type;
        uint8_t len;
        uint8_t bar0;
        uint8_t bar1;
    }
And there usually is a struct with the generic header only:
    struct Class {
        uint8_t type;
        uint8_t len;
    }

The len field references the struct itself. There is no need to sum up
all fields, lists and add padding to know the size of the struct. To
make use of this len field I proposed an additional <sizeof> element for
structs:
    http://lists.freedesktop.org/archives/xcb/2012-December/008038.html

With the <sizeof> element and further changes I made, those structs can
be implemented in the xml:
    <struct name="AnyClass">
        <field type="CARD8" name="type" />
        <field type="CARD8" name="len" />

        <sizeof>
            <!-- Will generate a _sizeof() function. -->
            <fieldref>len</fieldref>
        </sizeof>
    </struct>

    <struct name="FooClass">
        <field type="CARD8" name="type" />
        <field type="CARD8" name="len" />
        <field type="CARD8" name="num" />

        <list type="CARD16" name="numnum">
            <fieldref>num</fieldref>
        </list>
    </struct>

    <struct name="BarClass">
        <field type="CARD8" name="type" />
        <field type="CARD8" name="len" />
        <field type="CARD8" name="bar0" />
        <field type="CARD8" name="bar1" />
    </struct>

    <union name="Class">
        <!-- The generator will walk through the fields in the union and
             pick up the first <sizeof>. This one will be used to know
             the size of the fields when iterating over a list of the
             union. -->
        <field type="AnyClass" name="any" />
        <field type="FooClass" name="foo" />
        <field type="BarClass" name="bar" />
    </union>

Without this patch, foo would become a pointer. Which is wrong:
    typedef union xcb_something_class_t {
        xcb_something_any_t  any; /**<  */
   -    xcb_something_foo_t *foo; /**<  */
   +    xcb_something_foo_t  foo; /**<  */
        xcb_something_bar_t  bar; /**<  */
    } xcb_something_class_t;


As there are a lot of those unions in XI* vs. one in xkb (which might be
broken anyways) I would balance towards introducing this patch.

Josh, is your Reviewed-by still valid after this?


Cheers,
    Daniel Martin