Re: [Devel] Integer issues with FreeType

[Tom Kacvinsky]

Hi,

NOTE:  This was written after the response was composed (a post script in the 
pre
script, as it were).  You will find that I get snippier and snippier.  Everytime
I bring this topic up for discussion, I feel as if there is an attitude that 
there
really isn't a "problem" with the code in terms of how integer types are used.
I have come to the realization that I *strongly* disagree with that.  I believe
*exact* width integer types should *always* be used, to avoid unnecessary casts 
and
bitshifting, not to mention the fact that it makes it clearer what is being done
in the code.

I apologize in advance if this offends anyone, but I really do think that this 
will
help us avoid problems in the future.  And I am willing to do the work, so its 
me
that gets to wade through all the compiler warnings and weird results that will
ensue from this change I feel is necessary.

On Thu, 19 Apr 2001, David Turner wrote:

>
> Tom Kacvinsky a écrit :
> >
> > I plan on spending time this weekend going through the FT2 code base and
> > addressing the 64 bit long issue.  Before I do that, however, I want to get 
> > some
> > ideas from people about this.  Here is what I propose:
> >
> > The FT_* and FT_U* integer types are defined in fttypes.h, but only after
> > ft2build.h is included.  This means we get (through a chain of includes) the
> > FT_IntN (and FT_UIntN) definitions from ftconfig.h.  Thus, we ought to be
> > defining FT_Long and FT_Int, etc... in terms of FT_Int32, FT_Int16, etc...
> > This is a first step torwards removing FT_Long, FT_Int, etc... all together
> > and using FT_IntN and FT_UIntN instead (which makes it very clear what size
> > the values are supposed to be).
> >
>
> Why would you want to remove these, they're perfectly legal..
>

As I said, I know they are perfectly legal, but one has no idea whatsoever from
looking at the code base whether the values being read in are 8, 16, or 32 bits
wide.  Either we clarify in the headers that FT_Long is meant for reading 32 bit
quantities (and hence FT_Long is a 32 bit int), FT_Short is meant fo reading 16
bit quantities (and hence should be 16 bits wide, etc...), or we use FT_intN
intger type typedefs.

> > But what the conundrum for me is that there is no documentation of what 
> > integer
> > size FT_Long, FT_Int, FT_Short, etc... are supposed to be.
> >
> The sizes defined by the ANSI C specification, which is:
>
>   FT_Short  == short  == 16-bits
>   FT_Int    == int    == at least 16 bits, can be more (i.e. 24 bits, 32 
> bits, etc..)
>   FT_Long   == long   == at least 32 bits, can be more (i.e. 36 bits, 64 
> bits, etc..)
>

Yes, I know what the sizes are from the ANSI specification.  My question was
(and still is) this: from a *FreeType* standpoint, what is FT_Long supposed to
be used for, and how wide is it supposed to be ?  Now, I know from experience in
the code base that FT_Long is used for reading in or storing 32 bit quantities,
but that isn't in the documentation or the header files, is it?  This isn't
strictly for me and my selfish goals, it is for others. :)


> You should use the FT_IntXX and FT_UIntXX types only when you
> absolutely need an integer of an exact size. This is generally
> used when you're dealing with the structure of integers in
> memory, or when you need to perform exact wrapping, like in
> "((FT_UInt16)(x + 16))"..
>

This is where I believe I have disagreed with you in the past. :) I believe that
one should *always* use the exact size needed.  Otherwise, we get into
unnecessary bitwise operations.  Unnecessary in the sense that we don't have to
these kinds of things when storing a 32 bit signed quantity in 64 bit integer:

  i = (j << 32) >> 32.

We just set i knowing that the value on the rhs is 32 bits wide,a nd we are
done.


> However, even if you read a 32-bit signed value from a file, there
> is nothing wrong in storing it in a FT_Long (as long as you're not
> going to perform arithmetic on it, like incrementing/decrementing
> it).
>

Right, but this is one the things that needs a thorough going over.  Remember
the amount of bugs we caught with this in the CFF driver on 64 bit long
machines?

> Another source of problem is that the difference between pointers
> can be larger than a FT_Long on some platforms (64-bit ones indeed),
> which is why the FT_PtrDist type was introduced..
>

FT_PtrDist is one the things I plan on using. :)

> > So, what shall I do here?  Once I know what things are supposed to be, I can
> > work on adressing the issues at hand.  For instance, if FT_Int is supposed 
> > to be
> > 16 bits wide, there are several things that I will need to check in the
> > Type1/CID/CFF drivers so that changing FT_Int to be 16 bits wide won't break
> > things.
> >
> FT_Int isn't supposed to be a 16-bits int. You should not use it to
> perform arithmetic on 1--bit values however, unless you take care of
> casting everything appropriately..
>

Again, I am being pedantic, but the fact that FT_Int is not supposed to be 16
bits wide is not documented in fttypes.h.

> > I gather that the FT_* integer type definitions are based on the True Type
> > specification, so FT_Long is supposed to be 32 bits wide, and FT_Int is 
> > supposed
> > to be 16 bits wide (though I suppose that the TT spec uses short for this).
> > Etc, etc...
> >
>
> No, no, that's not correct, you're going to introduce lots of problem in the
> source code if you do so, not mentioning adding about 100K compilation 
> warnings :-)
>

OK, so now I know that FT_* integer types are not based on the TT spec.  Help me
understand the basis for using FT_Long, then.  Is it because the C89 spec. says
the int type needs to be at least 16 bits wide, whereas the long type is
guaranteed to be at least 32 bits wide, and we have need to read in 32 it
quantities?

> > Then there is the ftcalc.c functions that need to be addressed.  Ugh.
> >
> > Please do not not misunderstand me.  The code is working for me and many 
> > other
> > people, and I am not criticizing the code at all.  I just want to make the 
> > code
> > as portable as possible.  And this means I have to do things with macro
> > conditionals, because not all of the platforms FT runs on can use autoconf 
> > and
> > friends.
> >
>
> What do you mean exactly ?? I believe that the code in "ftcalc.c" is very
> portable already (it may not be a best performer for many platforms however).
> This code uses FT_Int32 and FT_UInt32 precisely because it's doing
> arithmetic on 32-bit values..
>

I didn't meant that there were bugs in ftcalc.c.  I meant that if I change what 
FT_Long
menas, then there will be some work to do ftcalc.c.

As for the portability issue: there is some work that needs to be done. I don't
consider code that core dumps (or gives erroneous results) on a machine that has
64 bit longs portable.  In fact, I have a complaint that my item in the TODO
list specifiying that 64 bit long issues needed to be addressed was removed.

>
> We could improve the FreeType source code slightly to detect 64-bit integers
> on a few compilers that support them like gcc, visual c++, etc.., without
> using Autoconf if you like, but this doesn't need the deep inspection you're
> mentioning..
>

Well, lets here it!

> > Anyway, the plan is for me to get another CVS scratch area set up on several
> > boxen that I have access to and test the changes locally.  I will then do a 
> > "cvs
> > diff -u -r" between two CVS trees and submit that to whomever wants to see 
> > it for
> > code review purposes.
> >
>
> Tom, I think you're heading for trouble.. What really needs to be checked
> are:
>
>   - arithmetic computations with 16-bit and 32-bit quantities
>   - use of pointer differences
>   - casts (to see if they're applied appropriately)
>

I would add that assigning 32 bit signed quantities to 64 bit signed ints needs
to be addressed.

> > Parting shot: all of this would be so much easier if every vendor out there 
> > had
> > C99 compliant C compilers.  Standards?  Hah!  We don't need no stinking
> > standards!
> >
> The old chicken and the egg problem. I'm really happy that we don't support
> K&R C syntax though :-)
>

True.

Tom