[Xcb] [PATCH proto 2/2] xinput: rep QueryDeviceState struct InputState: full support

[Christian Linhart]

Hi Daniel,

On 08/26/14 10:16, Daniel Martin wrote:

>> -    <struct name="InputState">
>> +    <struct name="InputState" implicit-padding="false">
> 
> Adding an explicit attribute to trigger implicit stuff where we
> could use an explicit <pad align> at the end of the struct or
> better: in the 2 cases where it's necessary doesn't sound
> right to me.
> 

The problem is that the implicit stuff is enabled by default.

Here, we disable it. 
So we cannot work around with adding <pad align>.

But I agree that it is not the ideal solution.

Alternatives are:
* alternative A:
  disable implicit padding in completely,
  except of the padding of C-structs by the compiler.

  Peter Harris said in another messaage of this thread
  that it can be completely disabled because it is a noop
  if the previous field is already aligned.
  And I agree with him.

  So it *should* be a noop everywhere.

  The problem is that testing and verification 
  can be a huge effort.

  The code for disabling implicit-padding can be derived
  from my patch for optionally disabling it:
  Keep only the code which is executed 
  when the member-variable "implicit_padding" is false.


* alternative B:
  fix the alignment within <switch>-constructs which start
  at an unaligned position within their struct.

  If this is not fixed, then the implicit-padding may
  generate wrong pads.

  This should be fixed anyways because it will also 
  break explicit align pads.

  But how to fix it?

  I can think of two solutions:

  solution 1:
    pass the padding-offset as an additional parameter to 
    functions like serialize, unpack, sizeof.
    This is not ideal because it breaks the API and makes
    the API more complicated.

  solution 2:
    derive the padding-offset from the pointer which is passed
    to functions like serialize, unpack, sizeof.

    This can be done approximately as follows, assuming the 
    the pointer is called "buffer":

    at the start of the function:
        int xcb_padding_offset = ((size_t)buffer) & 0x07


    replace:
        xcb_pad = -xcb_block_len & (xcb_align_to - 1);
    with
        xcb_pad = -( xcb_block_len + xcb_padding_offset ) & (xcb_align_to - 1);


    This will work without breaking the API but it
    depends on the protocal data being stored at an aligned position.
    This will generally be the case, so it should work.
    (But it is not really nice either.)

  What do you think?
  Which of these two fixes for the switch-align problem is better?
  Or is there another, even better fix?

***

Which alternative of 
* generally disabling implicit-padding
* or fixing the switch-alignment
should we choose?

Or should we choose both alternatives?

Chris