[Freeciv-Dev] Re: PATCH: check_map_pos

[Jason Dorje Short <vze2zq63@xxxxxxxxxxx>]

Gaute B Strokkenes wrote:
> 
> 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)

Sorry, I didn't mean an lvalue.  But the fact remains that if you make the
below function into a standard macro it will not fit in map_inx.

> > 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.

Sorry again :-)

You've haven't followed the full conversation, but the idea is that we
assert(0) in all the problem cases within check_map_pos.  That way we get
the core dump if we've compiled with debugging, and get corrected
coordinates if we've compiled without.  The freelog's are still useful for
non-debugging.

> >> > 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() ...

Yes, it is very doubtful that check_map_pos would be called with anything
other than valid identifiers.

> 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.

That's unfortunate.  I don't think bad performance with old compilers is a
problem, but not being able to compile at all is unacceptable.

>  * 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.

Yes, I have seen this myself.  However the difference is small.

>  * 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.diffName: NMP-gcc.diff
>                Type: Plain Text (text/plain)
> 
>   ----------------------------------------------------------------------------
> 
>    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.

So we are back to the beginning; trying to decide what checks are necessary
and what actions to take if a check is failed.

Checks are not needed everywhere; if a function just passes coordinates off
to another function/MACRO it is not necessary to do them.  A function/MACRO
that manipulates coordinates or uses them directly (like map_inx or
adjc_dir_iterate, for instance) should check the coordinates before acting
with them.  In other words, check_map_pos need only be called if you're
about to assume the coordinates are normal; this will mostly occur in
map_inx but not exclusively.  I believe we are all agreed on this?

If we're willing to have no checking when in NDEBUG mode, then we can get a
substantial speedup there just by

#define CHECK_MAP_POS(x, y) assert(is_normal_map_pos(x, y))

Personally, I would have no problem with this.  Since everyone using the CVS
version compiles with debugging, this will also suffice for our immediate
debugging purposes.  The problem is that in NDEBUG there's no checking, so
if there's a problem the game will probably be badly screwed up.  Using a
correctional system (like the one I proposed up top) would solve this, but
at a substantial cost in time.

Anyway, I'd be in favor of one of these two approaches.  Remember, it should
be easy to change later.

Finally, a reminder that the only remaining known problem is the focus hack
in the client.  If nobody knows what it is, can we ask Thue or another
off-list former developer?

jason