[Freeciv-Dev] Re: PATCH: check_map_pos

[Gaute B Strokkenes <gs234@xxxxxxxxx>]

On Sun, 21 Oct 2001, vze2zq63@xxxxxxxxxxx wrote:

>> > Fine.  Only two things: the macro can not be used inside map_inx,
>> 
>> Why not?
> 
> The macro aims at doing arbitrarily complex things like logging the
> descrepancy.  AFAIK, this is not possible within an l-value macro.

Why would you want or need it to be an lvalue?  The function you've
given below is not an lvalue.

#define map_inx(x,y) \
  (CHECK_POS((x),(y)), map.tiles + (x) + (y) * map.xsize)

> If it is, that would be great:
> 
> void check_map_pox(int *x, int *y)
> {
>   if (is_normal_map_pos(*x, *y))
>     return;
>   freelog(...);
>   if (normalize_map_pos(*x, *y))
>     return;
>   freelog(...);
>   abort(); /* or nearest_real_pos() */
> }

I do not think that we need this sort of bells and whistles.  If you
take the approach that each piece of code should be as forgiving as
possible, then that is basically what we have now.  When you are
debugging, a core dump is much more useful than a log message since
you can walk around the stack frame, inspect variables etc. etc.  Note
that with the above function even if you get a log message you have
absolutely no way of knowing _where_ the problem occurred, only that
it _has_ occured.  That makes its usefulness very limited.

>> > and map_inx is where the problems are most likely to occur.
>> > Also, I'd prefer to keep the name check_map_pos (instead of
>> > check_map_pos); the macro can ensure that passed parameters are
>> > not evaluated more than once.
>> 
>> You're not coming across here.  check_map_pos instead of
>> check_map_pos?
> 
> Oops.
> 
> I meant I would prefer check_map_pos to CHECK_MAP_POS.  We can make
> sure arguments are evalutated exactly once, if need be.

That's unlikely to be necessary.  Arguments to macros not being
evaluated exactly once tends to be a problem and cause bugs only when
you're not aware of it.  In pratice, there is seldom a case where this
causes difficulties.  I know this because I went through the sources
and checked this for is_real_tile(), normalize_map_pois() ...

There are a few uncomfortable facts that bear pointing out:

 * static inline is not portable.  You cannot just add it to the
   sources without taking the time to add the corresponding autoconf
   magic; even then you have to contend with unacceptable slow
   performance when using compilers that do not support it.

 * GCC does not generate code of equal quality when you use inline
   functions instead of macros.  I have posted profiling results that
   show this before.  Another way to see this is to inspect the
   generated assembly directly.

 * Even when you're using macros, GCC does not generate equivalent
   code for macros that take pointers rather than just the name of
   variables that they wish to modify.  To see what I mean, try
   applying the following patch to cvs from 2001-10-14:

NMP-gcc.diff
Description: Text document

   If you add "*(&(x or y))" around the arguments to NORMALIZE_POS()
   then the finished product is substantially slower, as can easily be
   verified by profiling or by inspecting the generated assembler
   code.

This is not necessarily a big deal in this case, since we are going to
disable the check entirely by default.  However it bears keeping in
mind if you wish to understand why the current, broken approach is so
slow.

-- 
Big Gaute                               http://www.srcf.ucam.org/~gs234/
..  My vaseline is RUNNING...