[Freeciv-Dev] Re: (PR#6257) recodify map_pos<->index conversion to use native positions

["Jason Short" <jdorje@xxxxxxxxxxxxxxxxxxxxx>]

rwetmore@xxxxxxxxxxxx wrote:
> Jason Short wrote:

>>   array[map_pos_to_index(x, y)] = array[map_pos_to_index(x1, y1)]
>>
>>is not legal C (just like a ^= b ^= a ^= b). 
> 
> 
> Please explain what is illegal in your map_to_index_pos() macro?
> 
> Of course a better way to code the above line to avoid issues surrounding
> execution order and overwriting of globals in a global form of the macro
> is to cache the index computations in local storage to guarantee sequential
> execution order. Option 4 should also help by decoupling the instances.

I believe the code violates ANSI C rules about sequence points

   http://www.eskimo.com/~scs/C-faq/q3.8.html

although I am far from an expert on this.  Recent versions of gcc give a 
warning when compiling it (see below).  So compiling with the attached 
patch (which takes the macro form practically straight out of the 
corecleanups) gives me

   mapgen.c: In function `mapgenerator5':
   mapgen.c:2145: warning: operation on `_FC_MAP_Y' may be undefined
   mapgen.c:2145: warning: operation on `_FC_MAP_X' may be undefined
   mapgen.c:2147: warning: operation on `_FC_MAP_Y' may be undefined
   mapgen.c:2147: warning: operation on `_FC_MAP_X' may be undefined
   mapgen.c:2158: warning: operation on `_FC_MAP_Y' may be undefined
   mapgen.c:2158: warning: operation on `_FC_MAP_X' may be undefined
   mapgen.c:2160: warning: operation on `_FC_MAP_Y' may be undefined
   mapgen.c:2160: warning: operation on `_FC_MAP_X' may be undefined
   mapgen.c:2177: warning: operation on `_FC_MAP_Y' may be undefined
   mapgen.c:2177: warning: operation on `_FC_MAP_X' may be undefined

which is not so good.

The argument that it is "better" to use local storage to manually create 
a sequence point is technically flawed.  This error is caused entirely 
by a bad implementation of the macro; it's rediculous to ask the callers 
to adapt to it.

jason

Index: common/map.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/map.c,v
retrieving revision 1.147
diff -u -r1.147 map.c
--- common/map.c        2003/10/02 17:54:57     1.147
+++ common/map.c        2003/10/03 19:00:34
@@ -34,6 +34,8 @@
 /* the very map */
 struct civ_map map;
 
+int _FC_MAP_X, _FC_MAP_Y;
+
 /* these are initialized from the terrain ruleset */
 struct terrain_misc terrain_control;
 struct tile_type tile_types[T_LAST];
Index: common/map.h
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/map.h,v
retrieving revision 1.159
diff -u -r1.159 map.h
--- common/map.h        2003/10/02 17:54:57     1.159
+++ common/map.h        2003/10/03 19:00:34
@@ -250,21 +250,11 @@
       *(pnat_x) = (2 * (map_x) - *(pnat_y) - (*(pnat_y) & 1)) / 2)          \
    : (*(pnat_x) = (map_x), *(pnat_y) = (map_y)))
 
-/* Use map_to_native_pos instead unless you know what you're doing. */
-#define map_pos_to_native_x(map_x, map_y)                                   \
-  (topo_has_flag(TF_ISO)                                                    \
-   ? (((map_x) - (map_y) + map.xsize                                        \
-       - (((map_x) + (map_y) - map.xsize) & 1)) / 2)                        \
-   : (map_x))
-#define map_pos_to_native_y(map_x, map_y)                                   \
-  (topo_has_flag(TF_ISO)                                                    \
-   ? ((map_x) + (map_y) - map.xsize)                                        \
-   : (map_y))
+extern int _FC_MAP_X, _FC_MAP_Y;
 
-#define map_pos_to_index(map_x, map_y)        \
-  (CHECK_MAP_POS((map_x), (map_y)),           \
-   (map_pos_to_native_x(map_x, map_y)         \
-    + map_pos_to_native_y(map_x, map_y) * map.xsize))
+#define map_pos_to_index(X,Y) \
+( map_to_native_pos(&_FC_MAP_X, &_FC_MAP_Y, (X), (Y)),                        \
+  map.xsize * _FC_MAP_Y + _FC_MAP_X )
 
 /* index_to_map_pos(int *, int *, int) inverts map_pos_to_index */
 #define index_to_map_pos(pmap_x, pmap_y, index) \