[Freeciv-Dev] Re: [PATCH] check_map_pos change (PR#1031)

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Fri, Oct 26, 2001 at 01:10:17AM -0700, jdorje@xxxxxxxxxxxxxxxxxxxxx wrote:
> The attached patch implements and uses a macro called check_map_pos(&x,
> &y) to check the coordinates (x,y) for normalness.
> 
> The idea, of course, is to avoid expensively calling normalize_map_pos
> at the start of every function, as is done now.  This patch replaces
> many such checks with a check_map_pos call.  The purpose of this call is
> three-fold: to catch any non-normal positions being handed around (which
> would be a bug), to optionally fix these positions so as to avoid a
> crash, and to be as fast as possible.
> 
> The current #definition of check_map_pos is
> 
> #define check_map_pos(x,y) assert(is_normal_map_pos(&x, &y))
> 
> which accomplishes goals #1 and #3.  It's a bit clunky, but should be
> good to start out with.
> 
> Not all normalize_map_pos calls need to be replaced by check_map_pos. 
> Ultimately, only those that do direct calculations with the coordinates
> need check_map_pos; those that just pass the coordinates on to another
> function do not.  To start with, it's probably safest to put
> check_map_pos in place of every normalize_map_pos and then scale back. 
> The only time I've broken this rule is with the accessor functions in
> map.c which clearly call MAP_TILE/map_inx immediately.
> 
> Finally, I have not replaced all normalize_map_pos invocations.  Of
> course, some of them are necessary but there are a lot that are dubious
> (especially in the different GUI's of the client).  To make things
> simple, I've only made replacements I'm pretty sure of.
> 
> Everything seems to continue to run fine under this patch.  In debug
> mode, there should be no savings, but if you compile with NDEBUG things
> should be significantly faster (I haven't profiled this).  Things could
> be sped up substantially in DEBUG mode by having is_normal_map_pos not
> call normalize_map_pos, if desired.

I think that we agreed that instead of replacing normalize_map_pos,
is_real_tile and co with check_map_pos we just remove them and trust
that we would catch all bad position in an access method (one which
uses map_inx). Look at this problem from this point of view: now every
method only accepts normal map positions. Why should some method have
a check for this and some not. Because of historical reasons? 
No. Either all method have such a check or none. And I agree that none
is ok. If there are problems we can add extra checks. But we don't
expect problems ;)

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 Windows: Where do you want to go today?
 Linux: Where do you want to go tomorrow?
 BSD: Are you guys coming or what?