[Xcb] [PATCH proto 1/1] Replace valueparam with switch-bitcase in Xproto

[Christian Linhart]

Hi Vincent,

Thank you very much for spotting this.

In fact, the 2 byte pad is still there but at the wrong position.
Therefore it needs to be removed from the wrong position.
I have added my comment below.

@Jaya: Can you please make a corrected patch?

Thanks,

Chris

P.S.: there is a protocol spec that is clearer than the one you have cited:
http://cgit.freedesktop.org/xorg/proto/xproto/tree/specs/encoding.xml#n1105


On 01/16/15 11:31, Vincent Chen wrote:
> Hi,
>
> I think there might be an error with the specification of
> ConfigureWindow. According to:
>
> http://www.x.org/releases/X11R7.7/doc/xproto/x11protocol.html#requests:ConfigureWindow
>
> there should be a two-byte padding between the mask and the values.
> The old valueparam with mask type of CARD16 (as opposed to the usual
> CARD32) did not make this need for extra padding explicit. With the
> new switch bitcase, however, the extra padding should be manually
> inserted.
>
> On Mon, Dec 22, 2014 at 10:35 AM, Jaya Tiwari <tiwari.jaya18 at gmail.com> wrote:
>> @@ -1689,11 +1816,40 @@ <enum name="StackMode">
>>    <request name="ConfigureWindow" opcode="12">
>>      <pad bytes="1" />
>>      <field type="WINDOW" name="window" />
>> -    <field type="CARD16" name="value_mask" />
>>      <pad bytes="2" />
This pad needs to be removed.
>> -    <valueparam value-mask-type="CARD16"
>> -                value-mask-name="value_mask"
>> -                value-list-name="value_list" />
>> +    <field type="CARD16" name="value_mask" mask="ConfigWindow" />
> Should add padding here:
>
> <pad bytes="2" />
Yes.
>
>> +    <switch name="value_list">
>> +        <fieldref>value_mask</fieldref>
>> +        <bitcase>
>> +          <enumref ref="ConfigWindow">X</enumref>
>> +          <field type="INT32" name="x" />
>> +        </bitcase>
>> +        <bitcase>
>> +          <enumref ref="ConfigWindow">Y</enumref>
>> +          <field type="INT32" name="y" />
>> +        </bitcase>
>> +        <bitcase>
>> +          <enumref ref="ConfigWindow">Width</enumref>
>> +          <field type="CARD32" name="width" />
>> +        </bitcase>
>> +        <bitcase>
>> +          <enumref ref="ConfigWindow">Height</enumref>
>> +          <field type="CARD32" name="height" />
>> +        </bitcase>
>> +        <bitcase>
>> +          <enumref ref="ConfigWindow">BorderWidth</enumref>
>> +          <field type="CARD32" name="border_width" />
>> +        </bitcase>
>> +        <bitcase>
>> +          <enumref ref="ConfigWindow">Sibling</enumref>
>> +          <field type="WINDOW" name="sibling" altenum="Window"/>
>> +        </bitcase>
>> +        <bitcase>
>> +          <enumref ref="ConfigWindow">StackMode</enumref>
>> +          <field type="CARD32" name="stack_mode" enum="StackMode"/>
>> +        </bitcase>
>> +    </switch>
>> +
>>      <doc>
>>        <brief>Configures window attributes</brief>
>>        <description><![CDATA[
> By the way, ConfigureWindow is the only place (that I know of) where
> the mask type is CARD16. Every other uses of valueparam uses mask type
> of CARD32. Not sure why the protocol designer didn't just make this
> CARD32 as well, since the mask plus the two padding uses up 4 bytes
> anyway...
>
> Regards,
>
> Vincent Chen
> _______________________________________________
> Xcb mailing list
> Xcb at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/xcb