[Freeciv-Dev] Re: (PR#8959) remove CAR_DIR_D[XY]

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

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

Found another way to iterate over the world to retrieve a subset of
information I see. You really do love this anti-pattern or have a
strong desire to write poor code into Freeciv.

But if you just coded in the efficient iterator core from any of the
corecleanups, you could change one value to loop over every second
element of the rotationally ordered adjacent array to get the cartesian
iterators - or just copy the existing code.

Really, all these useless changes for something that has been fixed for
a long time do not really appear to be a productive use of time, either
developer or runtime.

Doubling the iteration time for cartesian loops is clearly not a great
thing. And has been said, if you keep doubling execution times every
code spot you find, your program will soon run twice as slow. Hey, is
that not already the case?

Cheers,
RossW
=====

Jason Short wrote:

> <URL: http://rt.freeciv.org/Ticket/Display.html?id=8959 >
> 
> This patch removes CAR_DIR_DX and CAR_DIR_DY.
> 
> There are several reasons for doing this:
> 
> - It's cleaner; less code.
> 
> - The "cardinal" directions don't have to be hard-coded, allowing them 
> to be changed.  (Although it's possible to change the array at runtime 
> this method is much easier.)
> 
> The only drawback is it is potentially slower.  I don't have numbers on 
> this but I suspect the difference is unmeasurable (and I'm quite sure it 
> would be if DIR_IS_CARDINAL were inlined).  (The order of iteration may 
> be changed, perhaps causing a difference in savegames.  So it might be 
> hard to measure.)
> 
> jason
> 
> 
> 
> ------------------------------------------------------------------------
> 
> Index: common/map.c
> ===================================================================
> RCS file: /home/freeciv/CVS/freeciv/common/map.c,v
> retrieving revision 1.169
> diff -u -r1.169 map.c
> --- common/map.c      6 Jun 2004 20:45:27 -0000       1.169
> +++ common/map.c      11 Jun 2004 18:39:47 -0000
> @@ -42,10 +42,6 @@
>  const int DIR_DX[8] = { -1, 0, 1, -1, 1, -1, 0, 1 };
>  const int DIR_DY[8] = { -1, -1, -1, 0, 0, 1, 1, 1 };
>  
> -/* like DIR_DX[] and DIR_DY[], only cartesian */
> -const int CAR_DIR_DX[4] = {1, 0, -1, 0};
> -const int CAR_DIR_DY[4] = {0, 1, 0, -1};
> -
>  /* Names of specials.
>   * (These must correspond to enum tile_special_type in terrain.h.)
>   */
> Index: common/map.h
> ===================================================================
> RCS file: /home/freeciv/CVS/freeciv/common/map.h,v
> retrieving revision 1.187
> diff -u -r1.187 map.h
> --- common/map.h      6 Jun 2004 20:45:27 -0000       1.187
> +++ common/map.h      11 Jun 2004 18:39:48 -0000
> @@ -609,27 +609,13 @@
>  extern const int DIR_DX[8];
>  extern const int DIR_DY[8];
>  
> -/* like DIR_DX[] and DIR_DY[], only cartesian */
> -extern const int CAR_DIR_DX[4];
> -extern const int CAR_DIR_DY[4];
> -
> -#define cartesian_adjacent_iterate(x, y, IAC_x, IAC_y)                       
>  \
> -{                                                                            
>  \
> -  int IAC_i;                                                                 
>  \
> -  int IAC_x, IAC_y;                                                          
>  \
> -  bool _is_border = IS_BORDER_MAP_POS(x, y, 1);                              
>  \
> -  CHECK_MAP_POS(x, y);                                                       
>  \
> -  for (IAC_i = 0; IAC_i < 4; IAC_i++) {                                      
>  \
> -    IAC_x = x + CAR_DIR_DX[IAC_i];                                           
>  \
> -    IAC_y = y + CAR_DIR_DY[IAC_i];                                           
>  \
> -                                                                             
>  \
> -    if (_is_border && !normalize_map_pos(&IAC_x, &IAC_y)) {                  
>  \
> -      continue;                                                              
>  \
> -    }
> -
> -#define cartesian_adjacent_iterate_end                                       
>  \
> -  }                                                                          
>  \
> -}
> +#define cartesian_adjacent_iterate(x, y, center_x, center_y)             \
> +  adjc_dir_iterate(x, y, center_x, center_y, _dir) {                     \
> +    if (DIR_IS_CARDINAL(_dir)) {
> +
> +#define cartesian_adjacent_iterate_end                                      \
> +    }                                                                        
>     \
> +  } adjc_dir_iterate_end;
>  
>  /* Used for network transmission; do not change. */
>  #define MAP_TILE_OWNER_NULL   MAX_UINT8
> Index: server/mapgen.c
> ===================================================================
> RCS file: /home/freeciv/CVS/freeciv/server/mapgen.c,v
> retrieving revision 1.136
> diff -u -r1.136 mapgen.c
> --- server/mapgen.c   6 Jun 2004 20:45:27 -0000       1.136
> +++ server/mapgen.c   11 Jun 2004 18:39:48 -0000
> @@ -832,10 +832,9 @@
>      for (func_num = 0; func_num < NUM_TEST_FUNCTIONS; func_num++) {
>        int best_val = -1;
>        /* first get the tile values for the function */
> -      for (dir = 0; dir < 4; dir++) {
> -     int x1 = x + CAR_DIR_DX[dir];
> -     int y1 = y + CAR_DIR_DY[dir];
> -     if (normalize_map_pos(&x1, &y1)
> +      adjc_dir_iterate(x, y, x1, y1, dir) {
> +     if (DIR_IS_CARDINAL(dir)
> +         && normalize_map_pos(&x1, &y1)
>           && rd_direction_is_valid[dir]) {
>         rd_comparison_val[dir] = (test_funcs[func_num].func) (x1, y1);
>         if (best_val == -1) {
> @@ -844,22 +843,21 @@
>           best_val = MIN(rd_comparison_val[dir], best_val);
>         }
>       }
> -      }
> +      } adjc_dir_iterate_end;
>        assert(best_val != -1);
>  
>        /* should we abort? */
>        if (best_val > 0 && test_funcs[func_num].fatal) return FALSE;
>  
>        /* mark the less attractive directions as invalid */
> -      for (dir = 0; dir < 4; dir++) {
> -     int x1 = x + CAR_DIR_DX[dir];
> -     int y1 = y + CAR_DIR_DY[dir];
> -     if (normalize_map_pos(&x1, &y1)
> +      adjc_dir_iterate(x, y, x1, y1, dir) {
> +     if (DIR_IS_CARDINAL(dir)
> +         && normalize_map_pos(&x1, &y1)
>           && rd_direction_is_valid[dir]) {
>         if (rd_comparison_val[dir] != best_val)
>           rd_direction_is_valid[dir] = FALSE;
>       }
> -      }
> +      } adjc_dir_iterate_end;
>      }
>  
>      /* Directions evaluated with all functions. Now choose the best
> @@ -873,16 +871,15 @@
>      case 0:
>        return FALSE; /* river aborted */
>      case 1:
> -      for (dir = 0; dir < 4; dir++) {
> -     int x1 = x + CAR_DIR_DX[dir];
> -     int y1 = y + CAR_DIR_DY[dir];
> -     if (normalize_map_pos(&x1, &y1)
> +      adjc_dir_iterate(x, y, x1, y1, dir) {
> +     if (DIR_IS_CARDINAL(dir)
> +         && normalize_map_pos(&x1, &y1)
>           && rd_direction_is_valid[dir]) {
>         river_blockmark(x, y);
>         x = x1;
>         y = y1;
>       }
> -      }
> +      } adjc_dir_iterate_end;
>        break;
>      default:
>        /* More than one possible direction; Let the random number
> @@ -893,10 +890,9 @@
>        freelog(LOG_DEBUG, "mapgen.c: direction: %d", direction);
>  
>        /* Find the direction that the random number generator selected. */
> -      for (dir = 0; dir < 4; dir++) {
> -     int x1 = x + CAR_DIR_DX[dir];
> -     int y1 = y + CAR_DIR_DY[dir];
> -     if (normalize_map_pos(&x1, &y1)
> +      adjc_dir_iterate(x, y, x1, y1, dir) {
> +     if (DIR_IS_CARDINAL(dir)
> +         && normalize_map_pos(&x1, &y1)
>           && rd_direction_is_valid[dir]) {
>         if (direction > 0) direction--;
>         else {
> @@ -906,7 +902,7 @@
>           break;
>         }
>       }
> -      }
> +      } adjc_dir_iterate_end;
>        break;
>      } /* end switch (rd_number_of_directions()) */
>