Changing wl_output.transform type to unsigned?

[Nils Chr. Brause]

Hi,

On Tue, Nov 24, 2015 at 2:48 PM, Auke Booij <auke at tulcod.com> wrote:
> 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)

I am explaining in such detail, because this doesn't seem to be
obvious to everyone (otherwise it would already be a bitfield),
even though I think it is clear. But I can remove this line if this
helps to get the patch finally accepted. ;)

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

The actual bahviour is not part of this patch. I give these details, so
that people can understand, why wl_output.transform is a bitfield.
The exact behaviour can be documented in a separete patch.

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

Understood. I will try to change the message accordingly.

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

Thanks.

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

I will.

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

Actually the reason behind changing int to uint is the change
from an enumeration to a bitfield. Therefore they are both linked
together rather tightly. I wouldn't know how to proerly word the
commit message for the int -> uint change if I split these changes.

Cheers,
Nils