[PATCH] Declare enumeration wl_output.transform as bitfield.

[Pekka Paalanen]

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.

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.

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.


Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 811 bytes
Desc: OpenPGP digital signature
URL: <http://lists.freedesktop.org/archives/wayland-devel/attachments/20151109/eec0d9b2/attachment.sig>