Changing wl_output.transform type to unsigned?

[Nils Chr. Brause]

On Mon, Nov 16, 2015 at 11:46 AM, Pekka Paalanen <ppaalanen at gmail.com> wrote:
> On Sun, 15 Nov 2015 22:17:38 +0100
> "Nils Chr. Brause" <nilschrbrause at gmail.com> wrote:
>
>> On Sun, Nov 15, 2015 at 9:48 PM, Auke Booij <auke at tulcod.com> wrote:
>> > On 9 November 2015 at 18:17, Bill Spitzak <spitzak at gmail.com> wrote:
>> >> I really do not see the problem with allowing it to be an int argument as
>> >> long as the enum value 2^31 is not used. Though I am also stumped as to why
>> >> you can't change the current misused ints into uint in the protocol. It will
>> >> not change the bit layout in the messages and therefore is not a protocol
>> >> change.
>> >
>> > I don't really know what to do with this final claim. I like the idea,
>> > and it makes sense. Finally, it will solve this issue and potentially
>> > future ones as well. Is there any chance it could be implemented or is
>> > it a crazy idea?
>>
>> Bill is absolutely right. And it also doesn't even really change the C API,
>> because nobody is passing negetive numbers or number greater than 2^31-1
>> there anyway. Therefore, I am all for a change. :)
>
> Hi,

Hi,

sorry I was not answering in a while.

> your logic seems sound at first.
>
> The things we would need to change in the protocol are:
> - wl_surface.set_buffer_transform
> - wl_output.geometry
> - (possible third party extensions)
>
> It would break existing bindings for strongly typed languages that do
> not allow implicit conversion between signed and unsigned. (Does Java
> fall into that category?)

Strongly typed bindings would use special types for enumerations/bitfields
anyway, so this probably isn't a problem.

> You don't see any change on the wire, but changing the type changes the
> C API, which then changes types of variables used in C programs. I
> can't imagine this having an impact in this particular case, though.

The changes in the C API  doesn't even produce any warings. Take a look
at the following:

#include <stdint.h>
int foo(uint32_t c)
{
  return c;
}
int main()
{
  int32_t bar = 7;
  return foo(bar);
}

It compiles without any warings, even with -Wall. Tested an all available C
and C++ Standards with GCC 5.2.

>
> Weston seems to use mostly 'enum wl_buffer_transform' as the type, but
> struct weston_buffer_viewport already uses uint32_t.
>
> Ok, I suppose we could try this.
>
> The next step would be for someone to propose a patch to change the
> ints to uints in wayland.xml. Special attention should be given to the
> commit message: why change this, what benefits it gives, and even
> though it is breaking the protocol, why it cannot break anything in
> practice.
>
> It is important to write a good commit message, because I expect people
> to be looking at it a lot, since it is changing stable interfaces.

I tried my best to come up with an extensive commit message that should
explain everything:


Declare enumeration wl_output.transform as bitfield.

The enumeration wl_output.transform is clearly a bitfield.
The definition of a bitfield is that each bit has a distinct
meaning. This is clearly the case in the enumeration
wl_output.transform:

- bit 0: rotate by 90 degree
- bit 1: rotate by 180 degree
- bit 2: flip around vertical axis

Every other value can be constructed by ORing together the
above bits:

- "270" = "90" | "180"
- "flipped_90" = "90" | "flipped"
- "flipped_180" = "180" | "flipped"
- "flipped_270" = "90" | "180" | "flipped"

Therefore the bitfield="true" attribute has been added to
the enumeration declaration.

Also the argument type of each usage of wl_output.transform
has bee modified to be an uint to comply with the scanner
rules. This is a valid change, beause:

1. It does not change the wire protocol. Since bitfields do
   not have any numerical meaning, it doesn't make any
   difference wether they are transferred as signed or
   unsigned integers over the wire.
2. It does not change the ABI. On the ABI level, values are
   passed to function calls via registers or on the stack
   (depending on the architecture and calling convention)
   and neither argument passing method cares about the
   signdness of the passed arguments.
3. The API change is negligible. Every request that accepts
   this bitfield expects a number in the range of zero to
   seven. These numbers can be represented in and converted
   between signed and unsigned 32 bit integers without any
   loss and withiut any compiler warning (Tested with GCC
   5.2 and all available C and C++ standards).


If you like it, I can send a separate patch. If you want me
to change anything, just say and I will try to correct it.

>
> We'll see how that patch is received. If anyone complains it breaks
> their thing, I think we have to revert it, because it is technically
> breaking the stability rules.
>
>
> Thanks,
> pq

Cheers,
Nils