[PATCH] Declare enumeration wl_output.transform as bitfield.

[Nils Chr. Brause]

Hi,

On Mon, Nov 9, 2015 at 2:49 PM, Pekka Paalanen <ppaalanen at gmail.com> wrote:
> On Mon, 9 Nov 2015 12:54:00 +0100
> "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
>
>> >> >> 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.
>
> Hi,
>
> my motivation comes from C.
>
> A single-bit signed bitfield can have values 0 and -1, not 1, which is
> somewhat surprising. This is a small reason that promotes the
> convention that all bitfields should be unsigned. Of course, this is
> the C bit field feature, which is not very relevant here, except just
> having the same name.
>
> Another reason I can think of is conversions between different sized
> types. Signed values usually undergo sign extension, which can be a
> surprise when you are only thinking about bits.
>
> Maybe the most relevant ones are this:
>
>         "Right shift of a negative signed number has
>         implementation-defined behaviour."
>
> and:
>
>         "A left shift, if the number either starts out negative, or the
>         shift operation would shift a 1 either to or beyond the sign
>         bit, has undefined behaviour (as do most operations on signed
>         values which cause an overflow)."
>
> - http://stackoverflow.com/questions/4009885/arithmetic-bit-shift-on-a-signed-integer
>
> I wasn't really even aware of these two until I looked them up.
>
> For me all this belongs in the same category as the convention to
> always use signed types when you are going to do arithmetics, rather
> than using unsigned types only sometimes when you know you cannot have
> negative values for certain input variables but do have negatives for
> others. Mixing signed and unsigned types in arithmetic is error prone,
> so the convention is there to let programmers think less and also avoid
> compiler-specific or undefined behaviour.
>
>
> Thanks,
> pq

While I can see your motivation behind having bitfields only in a
unsigned integer,
none of your concers are actually relevant here, because the wl_output.transform
bitfield is already a signed integer. Therefore a C programmer already has to be
aware of the issues you have mentioned. Also I am not changing the C API.

This patch improves the documentation as it marks every bitfield as such
and it improves the usability of non-C language bindings. :)

Cheers,
Nils