bulia byak wrote:

Why "better"? What if one day, we need 64 bit precision - do a global search and replace again? The NR* stuff was there exactly for this: a layer of abstraction that allows to change the actual precision quickly - and it was not "a bug" because the typedefs are there for exactly this purpose (and they were not "general purpose" becaue the NR prefix limited their scope pretty narrowly to the NR subsystem).

Well, it's "better" because of fixing clarity in the code.

NRShort sounds like it's supposed to be a short of some type. It sounds like it's supposed to be the 'general' idea of a short.

Thus, it sounds like it was exactly intended to get a common, predetermined precision regardless and isolated from the underlying C implementation. And looking into gen_nr_config.c, it seems that is the case:

if (sizeof (short) == 2) { printf ("typedef signed short NRShort;\n"); printf ("typedef unsigned short NRUShort;\n"); } else { die ("sizeof (short) != 2"); }

Since there's a hardcoded check for a size of 2 (even after your update), it seems to clearly be saying "Hey! I want this NRShort type to be exactly 16-bit"

And then looking to the header at the begining, the 'kludge' factor goes up more:

/** * A little utility function to generate header info. * * Yes, it would be possible to do this using more "native" autoconf * features, but I personally find this approach to be cleaner. * * The output of this program is generally written to art_config.h, * which is installed in libart's include dir. **/

Anyway, with all the signs in the code, it seemed to be calling out for '16-bit' as the original intent. If that has changed, then we should tweak it. Yes, changing it to 'int' would work for most (but not all) systems, however that seems to be just a clever work-around to the true precision problem.

And... here we hit the crux of a problem. A simple work-around got the bug to go away. However, the clarity of the code was not updated, so later work undid the earlier fix. This is why I follow advice of Knuth and the like and consider clarity to be the first priority for code.

On Wed, 2003-12-31 at 17:49, bulia byak wrote:

Why "better"? What if one day, we need 64 bit precision - do a global search and replace again?

Well... that's a good point, actually. gint32 isn't necessarily the best solution either. Actually it's a bad one because it leaves the search and replace necessary, but makes it marginally harder. More on that later.

However, my understanding was that NRLong and NRShort were actually intended to be specific sizes -- that was the original purpose of the gen-nr-config business in the first place; to find (by compiling and running a test program) what types were 16 or 32 bits or whatever on the current platform.

Otherwise C's native long or short could have been used directly.

Of course, I don't recall that real size checks were ever implemented, so they just ended up being long/short anyway. Go figure. :P

Now, as far as the best solution... I think what we really need are single types that are used specifically for integer and floating-point measurements/coordinates.

Note that I said single types. Making NRShort and NRLong the same only completely fixes things while they're guaranteed to be the same precision (in which case, why have separate types?).

The zoom bugs and many others we've had to deal with have been the result of mixtures of precisions (or simply just not enough precision) used, in the name of micro-optimizing memory usage.

Also, that way, if we need more precision later, we can just bump it up in that one type rather than having to rework a bajillion things using different precisions.

Off the top of my head, we could maybe have NR::Coord (for floating-point) and NR::ICoord (for integer). Any other suggestions?

-mental