[Freeciv-Dev] Re: (PR#8773) rewrite city_map_iterate

["rwetmore@xxxxxxxxxxxx" <rwetmore@xxxxxxxxxxxx>]

<URL: http://rt.freeciv.org/Ticket/Display.html?id=8773 >

There is a patch cityclean.zip from Jan '01 that does a better job on
these iterators and in fact will integrate better with the direction
being followed by the generalized citymap patch.

Making these sorts of oneoff changes to slow down the code is really
not productive even though it is obviously the current policy and
continuation of the last few years fracturing and degradation of the
established codebase rules for as yet unexplained reasons.

As indicated by the continued apology for overriding any performance or
other good programming practices, the value really should be taken into
consideration, and/or a comprehensive review and correction of all the
instances be done instead.

As far as the current change goes, moving to a single loop is not a
bad thing, but replacing a dynamic expensive check with a simple cached
lookup as in the cityclean.zip might be all that is needed to port the
advantageous elements from the earlier patch. Put simply, lookup the
x and y values using the index coordinate in an array rather than using
arithmetic to compute them and a function call to filter them. I realize
the performance improvement from this will be hard to stomach, but maybe
just this once you could go for the good programming practice and forgo
the apology since the current fix is so close and just needs a minimal
amount of straightforward rework to overcome its flaws :-).

With this change, the patch becomes worthy of application.

Cheers,
RossW
=====

Jason Short wrote:

> <URL: http://rt.freeciv.org/Ticket/Display.html?id=8773 >
> 
> This patch rewrites city_map_iterate:
> 
> - It respects break (see PR#7075).
> - It uses is_valid_city_coords rather than a hard-coded check (see 
> PR#7350, PR#7481).
> 
> Those who would ask how much it slows down the code to use 
> is_valid_city_coords here, should instead ask how much it will speed up 
> the code to inline/macro-ise is_valid_city_coords.
> 
> jason
> 
> 
> 
> ------------------------------------------------------------------------
> 
> ? eff
> ? flags
> ? data/flags
> Index: common/city.h
> ===================================================================
> RCS file: /home/freeciv/CVS/freeciv/common/city.h,v
> retrieving revision 1.141
> diff -u -r1.141 city.h
> --- common/city.h     22 Apr 2004 22:58:28 -0000      1.141
> +++ common/city.h     19 May 2004 01:25:43 -0000
> @@ -84,15 +84,17 @@
>  
>  /* Iterate a city map */
>  
> -#define city_map_iterate(x, y)                     \
> -{                                                  \
> -  int x, y;                                        \
> -  for (y = 0; y < CITY_MAP_SIZE; y++)              \
> -    for (x = 0; x < CITY_MAP_SIZE; x++)            \
> -      if (! ((x == 0 || x == (CITY_MAP_SIZE-1)) && \
> -          (y == 0 || y == (CITY_MAP_SIZE-1))) )
> -
> -#define city_map_iterate_end                       \
> +#define city_map_iterate(x, y)                                               
>     \
> +{                                                                        \
> +  int _itr;                                                              \
> +  for (_itr = 0; _itr < CITY_MAP_SIZE * CITY_MAP_SIZE; _itr++) {         \
> +    const int x = _itr % CITY_MAP_SIZE, y = _itr / CITY_MAP_SIZE;        \
> +                                                                         \
> +    if (is_valid_city_coords(x, y)) {
> +
> +#define city_map_iterate_end                                             \
> +    }                                                                        
>     \
> +  }                                                                      \
>  }
>  
>  /* Iterate a city map, from the center (the city) outwards */