[Freeciv-Dev] Re: [PATCH] city_map_iterate cleanup

["Ross W. Wetmore" <rwetmore@xxxxxxxxxxxx>]

PLEASE don't use normal user variables in iteration macros. Even with
the proliferation of weird and wonderful uniqueness prefixes and 
suffixes this is very bad coding form.

The accepted way to do this is to use single "_" in macros. In general
double "__" is reserved for OS macros. This clearly identifies the 
variable as a macro construct. Look in the linux kernel headers for
examples that follow the standards correctly.

Here is the corresponding macro from corecleanup_06a as an example.

/* Iterate a city map in checked real map coordinates */
#define city_map_iterate_checked(x0, y0, cx, cy, mx, my) {             \
  int _x0= (x0)-CITY_MAP_SIZE/2, _y0= (y0)-CITY_MAP_SIZE/2;            \
  city_map_iterate(cx, cy) {                                           \
    int my= cy + _y0, mx= cx + _x0;                                    \
    if(normalize_map_pos(&mx,&my)) {

#define city_map_iterate_checked_end \
    }                                \
  } city_map_iterate_end             \
   
}

and the backwards compatible update from corecleanup_06b (in 06b because
touching beloved constructs like this is presumably controversial :-).

/* Iterate a city radius in map coordinates; skip non-existant squares */
/*   for backwards compatibility */
#define map_city_radius_iterate(city_x, city_y, x, y) \
  city_map_iterate_checked(city_x, city_y, _cx, _cy, x, y)
#define map_city_radius_iterate_end \
  city_map_iterate_checked_end
   

It has the added advantage that the user can specify both the city and
corresponding map coordinates rather than peeking at the macro values.

The code using this is also in corecleanup_06a, and is considerably more
compact getting rid of a lot of the ugliness commented on below.

Cheers,
RossW
=====

At 09:54 AM 01/08/22 +0200, Raimar Falke wrote:
>On Wed, Aug 22, 2001 at 03:21:30AM -0400, Jason Dorje Short wrote:
>> This small patch changes about 20 lines, adding in another
>> city_map_iterate and some is_valid_city_coords calls and removing about
>> a dozen lines of code.  Unfortunately, it doesn't look like any of the
>> other city coordinate iterations can be replaced by city_map_iterate.
>
>Partly applied.
>
>> -#define map_city_radius_iterate(city_x, city_y, x_itr, y_itr) \
>> -{ \
>> -  int x_itr, y_itr; \
>> -  int MCMI_x, MCMI_y; \
>> -  for (MCMI_x=0; MCMI_x<CITY_MAP_SIZE; MCMI_x++) { \
>> -    for (MCMI_y=0; MCMI_y<CITY_MAP_SIZE; MCMI_y++) { \
>> -      if (! ((MCMI_x == 0 || MCMI_x == (CITY_MAP_SIZE-1)) \
>> -         && (MCMI_y == 0 || MCMI_y == (CITY_MAP_SIZE-1))) ) { \
>> -        y_itr = city_y + MCMI_y - CITY_MAP_SIZE/2; \
>> -        if (y_itr < 0 || y_itr >= map.ysize) \
>> -      continue; \
>> -    x_itr = map_adjust_x(city_x + MCMI_x - CITY_MAP_SIZE/2);
>> +#define map_city_radius_iterate(city_x, city_y, x_itr, y_itr)
      \
>> +{
      \
>> +  int _city_x = (city_x), _city_y = (city_y);
      \
>> +  city_map_iterate(_city_x_itr, _city_y_itr) {
      \
>> +    int x_itr = _city_x + _city_x_itr - CITY_MAP_SIZE/2;
      \
>> +    int y_itr = _city_y + _city_y_itr - CITY_MAP_SIZE/2;
      \
>> +    if (!normalize_map_pos(&x_itr, &y_itr)) continue;
>>  
>> -#define map_city_radius_iterate_end \
>> -      } \
>> -    } \
>> -  } \
>> +#define map_city_radius_iterate_end
      \
>> +  } city_map_iterate_end;
      \
>>  }
>
>I haven't applied this one because of the CITY_MAP_SIZE/2 thing.
>
>       Raimar
>
>-- 
> email: rf13@xxxxxxxxxxxxxxxxx
>
>