[Freeciv-Dev] Re: (PR#4710) introduce map topology check macros

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

I suspect this is unnecessarily verbose and non-generalized for something
that is supposed to be part of a gen-topology implementation.

   map_hasFlag(<one of the topology flag enums>)

is a single macro that does it all, will be just as easy to read in code
and doesn't hardcode specific topology features in a function name keeping
the API interface to a minimum and making it easy to update over time.

Minor style nit, but keeps things data-driven vs locking in hardcoded names.

Cheers,
RossW
=====

BTW:
nearest_real_pos() is one of those bad functions that needs a nasty warning
comment and deprecation. It converts potentially unreal coordinates to real
ones which can wreak incredible havoc (it has in the past). This can only be
done safely in places that really understand why they are doing this, and
thus this is a bad thing to code as a general use function, especially with
no warning. If you must preserve the map_adjust() behaviour that caused so
much initial trouble and thankfully *is* deprecated, at least add the
comments in passing here to its reborn form.

Jason Short wrote:
> The attached patch introduces three new macros: map_is_isometric(), 
> map_has_wrapx(), and map_has_wrapy().
> 
> These are used by the calling code to check these three boolean conditions.
> 
> map_is_isometric() - return TRUE iff the underlying map is isometric
> 
> map_has_wrapx() - return TRUE iff the map wraps in the X (E-W) direction
> 
> map_has_wrapy() - return TRUE iff the map wraps in the Y (N-S) direction
> 
> One user is also introduced: nearest_real_pos.  In the process this 
> function is cleaned up a little (using CLIP and normalize_map_pos as 
> helper functions).
> 
> Currently the macro return values are hard-coded as TRUE or FALSE.  This 
> means the compiled calling code should not be changed at all by using 
> these macros (unless it's changed in other ways, of course - e.g., the 
> cleanup in this case).  Once all code checks the values this can be 
> changed so that they check a map variable to get their return value.
> 
> jason
> 
> 
> 
> ------------------------------------------------------------------------
> 
> Index: common/map.c
> ===================================================================
> RCS file: /home/freeciv/CVS/freeciv/common/map.c,v
> retrieving revision 1.141
> diff -u -r1.141 map.c
> --- common/map.c      2003/07/23 13:46:03     1.141
> +++ common/map.c      2003/07/28 13:45:58
> @@ -1352,15 +1352,20 @@
>  **************************************************************************/
>  void nearest_real_pos(int *x, int *y)
>  {
> -  if (*y < 0)
> -    *y = 0;
> -  else if (*y >= map.ysize)
> -    *y = map.ysize - 1;
> -  
> -  while (*x < 0)
> -    *x += map.xsize;
> -  while (*x >= map.xsize)
> -    *x -= map.xsize;
> +  int nat_x, nat_y;
> +
> +  map_to_native_pos(&nat_x, &nat_y, *x, *y);
> +  if (!map_has_wrapx()) {
> +    nat_x = CLIP(0, nat_x, map.xsize - 1);
> +  }
> +  if (!map_has_wrapy()) {
> +    nat_y = CLIP(0, nat_y, map.ysize - 1);
> +  }
> +  native_to_map_pos(x, y, nat_x, nat_y);
> +
> +  if (!normalize_map_pos(x, y)) {
> +    assert(FALSE);
> +  }
>  }
>  
>  /**************************************************************************
> Index: common/map.h
> ===================================================================
> RCS file: /home/freeciv/CVS/freeciv/common/map.h,v
> retrieving revision 1.151
> diff -u -r1.151 map.h
> --- common/map.h      2003/07/23 20:24:36     1.151
> +++ common/map.h      2003/07/28 13:45:58
> @@ -180,6 +180,10 @@
>    struct map_position *start_positions;      /* allocated at runtime */
>  };
>  
> +#define map_is_isometric() (FALSE)
> +#define map_has_wrapx() (TRUE)
> +#define map_has_wrapy() (FALSE)
> +
>  bool map_is_empty(void);
>  void map_init(void);
>  void map_allocate(void);