[PATCH] Declare enumeration wl_output.transform as bitfield.

[Nils Chr. Brause]

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.

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.

>
>
> Thanks,
> pq

Cheers,
Nils