[Freeciv-Dev] Re: [PATCH] Map cleanups (PR#1208)

[Jason Short <vze2zq63@xxxxxxxxxxx>]

Raimar Falke wrote:

> On Tue, Jan 08, 2002 at 03:50:59AM -0500, Jason Short wrote:
>
>>Ross W. Wetmore wrote:

>>I've already shown one example of "saved" values.  In that example it
>>was necessary to save the values even for the real case (where normalize
>>return TRUE), since it was the absolute positions that were needed in
>>both cases.  To generalize this, all unreal coordinates are equal; they
>>are all unreal and useless for most or all calculations.  In almost all
>>cases, when we do need to do coordinate handling in the unreal case it's
>>because we need to work with the _absolute_ coordinates rather than with
>>the normalized coordinates.  In such cases, we _have_ to store the
>>absolute coordinates, and normalize the map coordinates separately.  It
>>may be tempting to use the normalized coordinate in the real case and
>>the absolute coordinates in the unreal case, but this is usually wrong.
>>
>>For a real-world example, here is an example of code that you might
>>think would benefit from what you are suggesting, but in fact does not:
>>http://www.freeciv.org/lxr/source/client/gui-gtk/mapview.c?v=cvs#L1130
>>
>>I've looked through all current freeciv code to try to find an example
>>that uses the coordinates after normalize_map_pos returns FALSE (so that
>>I could explain how it was wrong :-), but I cannot find one.  There may
>>be such a usage where it happens far from the actual invocation of
>>normalize_map_pos, but it appears that all current code is correct in
>>this manner, and introducing this into the definition of
>>
>
>>normalize_map_pos will not safe us anything.
>>
>
> The SAFE_MAPSTEP macro. You can do a s/SAFE_MAPSTEP/MAPSTEP/

You'll still have to cache the values.  It's not the pre-normalization
positions we need, it's the pre-MAPSTEPping ones.  So the only way to
unify these macros is if normalize_map_pos returns the nearest_real_pos
coordinates.

My miswriting of SAFE_MAPSTEP in the previous mail is probably confusing
you.  The correct way to write it is

#define SAFE_MAPSTEP(dest_x, dest_y, src_x, src_y, dir) \
    do {                                                 \
      DIRSTEP(dest_x, dest_y, dir);                     \
      (dest_x) += (src_x);                              \
      (dest_y) += (src_y);                              \
      if (!normalize_map_pos(&(dest_x), &(dest_y))) {    \
        (dest_x) = (src_x);                              \
        (dest_y) = (src_y);                              \
        CHECK_MAP_POS((dest_x), (dest_y));               \
      }                                                  \
    } while(0)


>>But while we're on the topic, here is an aside: there are a growing
>>number of calls of the form:
>>
>>   is_real = normalize_map_pos(&x, &y);
>>   assert(is_real);
>>
>>in such cases, having a non-real position means a serious bug, and
>>deserves the assertion.  But if we compile with NDEBUG we'll just
>>continue merrily on our way, which probably not a good idea (it'll lead
>>to segfaults and very unpredictible behavior).  It would be much safer
>>to accept the void-tilish fix and do something like
>>   {
>>     int _x = x, _y = y;
>>     if (!normalize_map_pos(&x, &y)) {
>>       assert(0);
>>       x = _x, y = _y;
>>       nearest_real_pos(&x, &y);
>>     }
>>   }
>>In fact, this is the only valid piece of code I can think of that uses
>>the (x,y) values of normalize_map_pos after it's returned FALSE.
>>Although it will certainly not guarantee any kind of correct behavior,
>>it will generally give many fewer segfaults and less unpredictable code
>>than without the nearest_real_pos call.  This _could_ also be written as
>>   if (!nearest_real_pos(&x, &y)) {
>>     assert(0);
>>   }
>>
>>if we change that function as you suggest.
>>
>
> Setting NDEBUG in the above case should gain speed. However you still
> do the test in your proposed code. So I'm more for a new assert macro
> which can't be disabled.

Well, if we're not sure the coordinates are normal (as we're not) then
we need to do the normalization for both DEBUG and NDEBUG.  Substituting
CHECK_MAP_POS in for these will allow us to gain speed (if we choose) in
the NDEBUG case.


>>In any case, there are currently 32 calls to normalize_map_pos that
>>assume the position is real.  Most use the first construct up above, but
>>a few use slower or even incorrect constructs instead (also, many of
>>them should use CHECK_MAP_POS).  I think a new macro,
>>normalize_real_map_pos would be appropriate to deal with this.
>>
>
> This is a optimization-only thing? How much percent of the runtime is
> used for normalize_map_pos if CHECK_MAP_POS is disabled?

I'm not concerned about the optimization, but about consistency,
readability, and correctness.  Making this a macro saves about 32x2
lines of code overall, and unifies the structures into one
sure-to-be-correct one.

jason