[Freeciv-Dev] (PR#9869) Bug: DDEBUG changes savegames

["Jason Short" <jdorje@xxxxxxxxxxxxxxxxxxxxx>]

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

> [glip - Mon Aug 30 17:06:31 2004]:
> 
> On Sun, 29 Aug 2004, Jason Short wrote:
> 
> > <URL: http://rt.freeciv.org/Ticket/Display.html?id=9869 >
> >
> > Gregory Berkolaiko wrote:
> >
> > > I am not sure which is the better way to fix this: do temporary
> > > assignments inside index_to* functions or do it in the caller.
> The first
> > > way might give more work to compiler (optimizing), the second will
> give
> > > more work to us (reviewing all uses).
> >
> > We can't do temporary assignments unless we make it an inline
> function.
> >   Changing the users is fine but we also have to rename it as
> > INDEX_TO_MAP_POS.  I would prefer the former but either is fine.
> 
> I am confused.  First, why cannot we do temp assignments in a macro
> (unless we want it as an lvalue or something)
> 
> #define blah(a, b) {int c = a; b += c; b *= c;}

You can, but then the macro cannot be used like a function; it cannot be
used in an expression.  I had assumed that some users would do this but
maybe they don't.

> Second, if we change the users, why should we rename it? (Or is it
> because
> of the new guidelines macros vs inlines?)

The guidelines.  We see in this case why they are needed - this macro
looks like a function but it's not.  If it evaluates arguments more than
once it should be CAPITALIZED.

Note that a _lot_ of macros don't satisfy this requirement. 
native_to_map_pos and map_to_native_pos may be the most dangerous ones.

> Third, which one you'd actually prefer, to make it inline?

I prefer making it inline since its sister function map_pos_to_index is
inline.  For the other conversion macros we should probably just
capitalize them.

> Fourth, why don't you make a patch ;)

Attached.  Is this sufficient to fix the DEBUG problem?

jason

Index: common/map.h
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/map.h,v
retrieving revision 1.210
diff -u -r1.210 map.h
--- common/map.h        26 Aug 2004 18:37:52 -0000      1.210
+++ common/map.h        31 Aug 2004 03:22:52 -0000
@@ -319,12 +319,7 @@
     : map.ysize)
   
 static inline int map_pos_to_index(int map_x, int map_y);
-
-/* index_to_map_pos(int *, int *, int) inverts map_pos_to_index */
-#define index_to_map_pos(pmap_x, pmap_y, index) \
-  (CHECK_INDEX(index),                          \
-   index_to_native_pos(pmap_x, pmap_y, index),  \
-   native_to_map_pos(pmap_x, pmap_y, *(pmap_x), *(pmap_y)))
+static inline void index_to_map_pos(int *map_x, int *map_y, int index);
 
 #define DIRSTEP(dest_x, dest_y, dir)   \
 (    (dest_x) = DIR_DX[(dir)],         \
@@ -694,6 +689,18 @@
 }
 
 /****************************************************************************
+  Convert an index position into a map position.  See doc/HACKING.
+****************************************************************************/
+static inline void index_to_map_pos(int *map_x, int *map_y, int index)
+{
+  int nat_x, nat_y;
+
+  CHECK_INDEX(index);
+  index_to_native_pos(&nat_x, &nat_y, index);
+  native_to_map_pos(map_x, map_y, nat_x, nat_y);
+}
+
+/****************************************************************************
   A "border position" is any map position that _may have_ positions within
   real map distance dist that are non-normal.  To see its correctness,
   consider the case where dist is 1 or 0.