Re: [RFC] Framebuffer rotation patch, or why 'unsigned' fails us

[Colin D Bennett]

On Sat, 13 Feb 2010 18:29:41 +0100
Michal Suchanek <address@hidden> wrote:

> 2010/2/11 Vladimir 'φ-coder/phcoder' Serbinenko <address@hidden>:
> > Michal Suchanek wrote:
> >> 2010/2/11 Vladimir 'φ-coder/phcoder' Serbinenko
> >> <address@hidden>:
> >>
> >>> Michal Suchanek wrote:
> >>> -    unsigned int x;
> >>> -    unsigned int y;
> >>> -    unsigned int width;
> >>> -    unsigned int height;
> >>> +    int x;
> >>> +    int y;
> >>> +    int width;
> >>> +    int height;
> >>> Why do you need negative values?
> >>>
> >>
> >> I don't need the negative values everywhere but this change was to
> >> align the typing so that casts are not required.
> >>
> >>
> > Perhaps few casts together with appropiate checks when casting is
> > better than potentially exposing the whole code to the values it's
> > not intended for.
> 
> How is it exposed to more unwanted values than now?
> It uses the variables only to store the viewport dimensions and at
> most adds a border around it. The unsigned integer is able to
> represent values outside of the viewport as much as signed integers
> are, only signed integer can represent values outside of viewport in
> both directions. The unwanted values (if erroneously calculated which
> does not seem to be the case here) are clipped by the viewport
> clipping in video_fb.
> 
> On the other hand, there would be more than a few casts as the
> variables are used multiple times and the interface now uses signed
> integers.

I have to chime in on this one since I've learned the hard way that
'unsigned' doesn't pull its weight in these use cases.  Using unsigned
integer values in C when a lot of arithmetic is performed (as in
graphics programming) is really a nightmare. Well, maybe if you are a
grand master C programmer you would remember when you need to cast and
never make a mistake, but remember that there are us mere mortals who
would benefit by more logical and predictable code behavior.  (I use
'predictable' not in the sense that expressions involving both signed
and unsigned values are have an undefined result, but rather that it's
usually not the obvious result that the average C programmer would
expect at first glance.

Certainly it would be nice if we could use the 'unsigned' qualifier to
enforce an invariant requiring only non-negative values be stored in
it (which it technically does, but it doesn't necessarily make the
result any more correct).  It almost never really fixes the kind of
problems we'd like it to.  We could easily try to store the result of a
subtraction that conceptually should produce a negative value into an
unsigned variable, and--OOPS!--we now have an enormous value instead of
a negative value. Not much better.  More significant is code that
involves multiple operations such that the result of the entire
expression should be nonnegative, but it involves temporary negative
values in subexpressions such that the implicit signed-unsigned
conversion produces the "wrong" result (i.e., not what the programmer
intended even though the expression looks correct).

Let me quote an entry from my gfxmenu project journal, 31 May 2008:

    It is worthwhile noting the potential perils of unsigned types in C
    (and C++).  I spent a lot of time trying to figure out why my
    apparently simple code was not working. It turns out that I had
    expressions that combined signed and unsigned integral types. The
    unsigned types were the viewport origin/size, which GRUB defines to
    be unsigned, and one of the signed types was an offset that was
    added to the bitmap position during the animation. This offset had
    to be signed since it could be either positive or negative,
    depending on the present direction of the bitmap's movement. The
    final solution was to use signed types only. I read some
    discussions on signed vs. unsigned types and found this quote from
    Bjarne Stroustrup, which in my experience is wise advice:

        The unsigned integer types are ideal for uses that treat storage
        as a bit array. Using an unsigned instead of an int to gain one
        more bit to represent positive integers is almost never a good
        idea.  Attempts to ensure that some values are positive by
        declaring variables unsigned will typically be defeated by the
        implicit conversion rules.

Anyway, that's my two cents after having been bitten by arithmetic
involving both signed an unsigned values.  And it's tedious to go check
the exact types of each value involved in an expression just to see
what casts you might need to produce the right result, especially when
the values involved are declared far away in some header file (i.e.,
structure members).

Regards,
Colin