[PATCH] Declare enumeration wl_output.transform as bitfield.

[Nils Chr. Brause]

Hi,

On Mon, Nov 9, 2015 at 3:39 PM, Auke Booij <auke at tulcod.com> wrote:
> On 9 November 2015 at 11:54, Nils Chr. Brause <nilschrbrause at gmail.com> wrote:
>> Hi,
>>
>> On Mon, Nov 9, 2015 at 12:04 PM, Pekka Paalanen <ppaalanen at gmail.com> wrote:
>>> On Fri, 6 Nov 2015 16:05:10 +0100
>>> "Nils Chr. Brause" <nilschrbrause at gmail.com> wrote:
>>>
>>>> Hi,
>>>>
>>>> On Fri, Nov 6, 2015 at 3:48 PM, Auke Booij <auke at tulcod.com> wrote:
>>>> > On 6 November 2015 at 13:03, Nils Christopher Brause
>>>> > <nilschrbrause at gmail.com> wrote:
>>>> >> 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
>>>> >
>>>> > Just to complete the definitions: what is the right order of
>>>> > operations here? How do I transform from "normal" to
>>>> > wl_output::transform = 0b101, for example? First rotating 90 degrees
>>>> > and then flipping is not the same as first flipping and then rotating
>>>> > 90 degrees. In mathematical terms, the symmetry group of the square is
>>>> > not a commutative group.
>>>
>>> Hi,
>>>
>>> for the record, what these values actually mean always totally confuses
>>> me even without the interpretation as a bitfield.
>>>
>>>> >> 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"
>>>> >
>>>> > While I agree with this, this does not seem to be used anywhere in
>>>> > weston's source code. Not saying I disagree with your claim that
>>>> > wl_output::transform should be considered a bitfield, but why is it
>>>> > necessary to do so?
>>>>
>>>> I think everything that is a bitfield should be markted as such. Even if
>>>
>>> Otherwise I'd agree, but in this case we have to add exceptions in the
>>> documentation, scanner, and language definition just because
>>> historically we happen to use int rather than uint here. Too bad, but
>>> in this case we have to live with not defining it a bitfield. I believe
>>> this is the lesser evil.
>>>
>>>> weston doesent make use of the bitfield nature of wl_output::transform,
>>>> users might want to do so. For example a user might want to check if
>>>> a particual transformation includes a flip. Instead of having to do four
>>>> comparisions (wich even might not be enough if more transformations
>>>> are added in the future), a single AND would suffice here.
>>>
>>> I find it hard to imagine any more cases to be added. These already
>>> cover all cases you can rotate a 2D-rectangle in 3D while maintaining
>>> axis-aligment. This geometry comes from the physical reality of how
>>> devices showing a 2D image work.
>>>
>>>> >> Therefore the bitfield="true" attribute has been added to
>>>> >> the enumeration declaration. Since this bitfield is
>>>> >> transferred as signed integer, the scanner had to be
>>>> >> modified to accept that behaviour. This was also noted in
>>>> >> the documentation.
>>>> >
>>>> > As I made clear in our previous discusions, I don't think that it is a
>>>> > safe idea to allow signed integers to be bitfields. Requiring
>>>> > unsignedness is a sanity check. So I would not like this patch, if
>>>> > only for the reason that it invalidates this check.
>>>>
>>>> I don't see why the signedness should even matter. A bitfield doesn't
>>>> have a numerical value after all. It is just a collection of bits.
>>>
>>> I totally agree with Auke here.
>>>
>>> Let's reject this patch. It's just one more historical accident we have
>>> to live with.
>>
>> I still got no explaination why the signdness of a bitfield that has no
>> numerical meaning even matters. (Rasons like "it feels natural" aside.)
>> It is totally irrelavant whether a collection of bits is marked as signed or
>> as unsigned. Therefore there is no sensible reason to reject this patch.
>
> I commented on this last time. But let me bring up another point: if
> everything that is just a collection of bits should be bitfield-able
> (following your previous logic of "everything that can, should" here),
> then so should the "fixed" type. And so "string" type (arguably it is
> even better suited for this purpose than uint, since it allows a
> practically unlimited number of flags, and if most of them are zero,
> it can still transfer them efficiently). And so should "object"
> (arguably it is even better than uint, because it is emphasized that
> it should not be interpreted as an integer or natural number).
>
> This is the mess that I would like to avoid. For funny types like
> those: well maybe you have a very good point in calling it a bitfield.
> But once you exit the realm of uint, we're in nonstandard bitfield
> terrain, and we cannot make any *systematic* promises from a scanner
> point of view. So although you're allowed to package bitfields in an
> int or a string or whatever you like, we cannot deal with these
> systematically, since the only thing that we *define* systematically
> is the uint case.

Would you be fine with having an exception for wl_output.transform
being added to the C scanner? That way the bitfield could be properly
marked as such and the scanner would still be able to enforce rules
on all the other bitfields as well as new ones.

>
>> Nevertheless, this enumeration will be a bitfield in the C++ bindings, even
>> if that means that I have to add an exception in the scanner.
>
> That is very reasonable. It's wl_output::transform that is the
> exception to the rule, and everyone agrees that this is purely for
> historical reasons: there is no deeper meaning behind it being an int,
> and it was a mistake, and it's one all bindings will have to deal with
> (if they deem it necessary) since the protocol is now stable.

Cheers,
Nils