[Freeciv-Dev] Re: PATCH: check_map_pos

[Jason Dorje Short <vze2zq63@xxxxxxxxxxx>]

Raimar Falke wrote:
> 
> On Mon, Oct 15, 2001 at 04:29:10AM -0400, Jason Dorje Short wrote:
> > The attached patch introduces a check_map_pos() function.
> >
> > It's done as an inline function within map.h, but this is unimportant at
> > this point.  As said before, the advantage of a function is that it can
> > be used within map_inx if desired; there's not really any advantage to a
> > macro.
> >
> > It does not touch client code.  With all the different GUI's and the
> > tendancy to pass around coordinates picked by the user, the client is
> > not yet ready for this function.
> >
> > I have run several full (endyear=2000) autogames, and have not had any
> > problems yet.
> >
> > Note that check_map_pos is not called on coordinates that are simply
> > passed off to another function; for instance most of the code in map.c
> > just calls MAP_TILE or map_inx with the coordinates so it's not
> > necessary to call check_map_pos directly.
> 
> So since the code base is ready for the change we have to discuss
> about the form.

Unfortunately the client isn't ready for this change.  It will fail the
assertion after a little while.

We can still make the change, if we remove the assert(0) in
check_map_pos.  That way there will be error messages, but no outright
problems.  It will also be easy enough to reinsert the assert() call to
get core files once problems are found.

> IMHO we should use macros to be able to completely
> disable this checks. IMHO it isn't a good idea to mix these two things
> (introduction of check_map_pos and inline).

Fine.  Only two things: the macro can not be used inside map_inx, 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.  This last will make it easier to convert to a function later if
desired.

Perhaps an intermediate step would introduce check_map_pos as a
non-inline function?  This would give a performance hit, but otherwise I
think we're going to have some ugliness here.

In the meantime I can try to track down the client problems, although
they may be GUI-specific.

jason