Changing wl_output.transform type to unsigned?

[Auke Booij]

On 24 November 2015 at 12:41, Nils Chr. Brause <nilschrbrause at gmail.com> wrote:
> 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.

(if this is true, then why do you continue explaining this clear fact?
either it's clear or it requires explanation, not both)

> 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

As I brought up previously: if you want to specify this, you might as
well go the extra mile and figure out which direction the rotation is
in, and if the flipping goes before or after the rotation. otherwise
you're not really saying anything people cannot figure out on their
own.

> 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.

I believe commit messages should be written in an imperative mood.

https://www.kernel.org/doc/Documentation/SubmittingPatches

> Also the argument type of each usage of wl_output.transform
> has bee modified to be an uint to comply with the scanner

* been

> 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.

(without suggesting that I am in any way an authority on this,)
perhaps it would be a good idea to send an [RFC] about this: that
makes this change more visible (which it should be).

>> 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

Thanks for picking this up.

I think it would be best to split this up into two commits (one int ->
uint, another bitfield="true"). And I think it is the former that
needs thorough explanation - your reasoning for the latter is clear,
and not a change that breaks a stable protocol (since adding
attributes that were previously unspecified, by definition, never
breaks protocol).