[PATCH] Declare enumeration wl_output.transform as bitfield.

[Bill Spitzak]

Making the transform into a bitfield allows bitfield tests for useful
facts: it can see if it is a mirror image by testing the flip bit, and
check for transposition of the axes by checking the 90 degree bit. I
believe this is the reason behind the desire to declare it a bitfield and I
agree this is nice to have.

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.


On Mon, Nov 9, 2015 at 7:51 AM, Nils Chr. Brause <nilschrbrause at gmail.com>
wrote:

> Hi,
>
> On Mon, Nov 9, 2015 at 4:35 PM, Pekka Paalanen <ppaalanen at gmail.com>
> wrote:
> > On Mon, 9 Nov 2015 15:38:22 +0100
> > "Nils Chr. Brause" <nilschrbrause at gmail.com> wrote:
> >
> >> 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.
> >
> > That is the very reason why it should not be marked as a bitfield.
> >
> > If you mark it as a bitfield, it encourages the use of bit-wise logic
> > ops. The data type is still signed. That is like knowingly luring
> > programmers into a trap.
>
> To be honest, I expect a programmer who wants to use the Wayland C API
> directly (rather than using QT or the like) to be smart enough to be aware
> of
> that. Any decent programmer would only use AND, OR, XOR and NOT with
> bitfields, which are toatally safe with signed integers.
>
> > You easily avoid the pitfalls by handling it as an enum, not a bitfield.
> >
> > This is also the rationale behind the decision to require that
> > bitfields must be unsigned. The convention goes both ways: bitfield
> > must be unsigned, and signed must not be a bitfield.
> >
> >> This patch improves the documentation as it marks every bitfield as such
> >> and it improves the usability of non-C language bindings. :)
> >
> > We can argue whether it is a bitfield or not. There is certainly no
> > requirement to handle it as a bitfield as all possible "combinations"
> > are easily enumerated: there are only eight, and they are all listed
> > explicitly. Furthermore, all bits are part of the same thing:
> > transformation. There are no bits that would not affect the thing you
> > compute from any of the other bits.
>
> In my definition of a bitfield, it is one. See above.
>
> > If we ever get the open/closed notation, this would be a closed enum.
> >
> > However, all the above argumentation pales when we look at what doing
> > what you suggest would require: either throwing away the requirement
> > for uint bitfields completely, or hardcoding an exception. To me, both
> > are far far too heavy solutions to a problem that is minor at most.
>
> While I do not agree with you, I will accept your decision and add an
> exception
> to the scanner of the C++ bindings to deal with this defect in the
> protocol XML.
>
> > Thanks,
> > pq
>
> Cheers,
> Nils
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/wayland-devel/attachments/20151109/71d91ae8/attachment.html>