[PATCH] Declare enumeration wl_output.transform as bitfield.

[Auke Booij]

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.

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