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

["Jason Short" <jdorje@xxxxxxxxxxxxxxxxxxxxx>]

rwetmore@xxxxxxxxxxxx wrote:
> Jason Short wrote:

> We have had a number of discussions about interface and implementation,
> and how a good programmer really should be able to look just at the
> interface and ignore the implementation when doing the design, since the
> interface if done properly lives through many implementation iterations,
> revisions and extensions.

Unfortunately these all tend to end in an impasse.  I think we disagree 
on what comprises the interface and what is the implementation.  See below.

>>   switch (get_maptype(MAP_TYPE_WRAPX | MAP_TYPE_WRAPY)) {
>>     case MAP_TYPE_FLAT:
>>
>>     case MAP_TYPE_WRAPX:
>>
>>     case MAP_TYPE_WRAPY:
>>
>>     case MAP_TYPE_TORUS:
>>
>>   }
> 
> 
> Again to repeat some of the old arguments for the list ...
> 
> This is implementation code which out of context has no real meaning, and
> could easily be coded in several different ways depending on your taste.
> It has little to do with interface design and nothing to how a programmer
> would use the above interfaces in *his/her* code :-).

I do not consider this implementation code.  The topology checks are 
their own little module, and general map code is not a part of their 
implementation.  These concepts should be kept separate.

> For example, I have been trying to get you to avoid defining types by
> ORing the implementation bits as in the above. One needs separate enums
> for Flag bits and basic types. It is also often better to use the switch
> argument in the above as an index into a data or function table. That is
> what C++ or object oriented languages tend to do.
> 
>    value = value_array[get_maptype(MAP_TYPE_TORUS)];
>      or
>    (*funct[MAP_TYPE_TORUS])(...);
> 
>    with TOPO_FLAG_WRAPX being something different and unrelated.
> 
>    Some of these tricks actually got into the PF code, so you can look
> for better ways to provide the glue between enums and implementations
> than code switches.

This is FUD.  The interface I proposed does nothing to interfere with 
implementing things this way.  You can still do whatever you want in the 
backend.

> But first you need to learn to keep the implementation bits out of the
> interface.

The types of flags available will be a part of the interface in any 
case.  Whether you do it as a new enumerated value or a new one-line 
macro, the callers have to be changed to use the new code.

>>My alternative is to keep the interface very simple - three macros 
>>returning only boolean values.  The backend interface can do whatever it 
>>wants - bitfields are the most likely since we aim to have a single 
>>value representing the topology ID.
> 
> 
> Even simpler is one macro returning a boolean value, the implementation
> bits etc. comments apply equally well :-).

No, this is not simpler.  Attached is an alternative implementation that 
does it your way.  I have no major objections to writing the interface 
this way, but it's certainly not any simpler.

My minor objection is that it's not clear what values will be returned 
if you use the interface in a nonstandard way.  For instance I've 
written the macro as

   #define topo_has_flag(flag) ((CURRENT_TOPOLOGY & (flag)) != 0)

which means that

   topo_has_flag(TF_WRAPX | TF_WRAPY)

returns TRUE if _either_ TF_WRAPX or TF_WRAPY is set.  But it could just 
as easily have been written as

   #define topo_has_flag(flag) ((CURRENT_TOPOLOGY & (flag)) == (flag))

in which case the above call would only return TRUE if wrapx and wrapy 
were _both_ true.

It also tempts you to define intermediate enumerated values like 
TF_TORUS or TF_ISO_TORUS, which just make it easier to make mistakes 
like the above one.  If we have an interface like this, each call should 
be done on a single flag.  Of course, using the macro form enforces this 
at the interface level.

> Unlike your case which will add an indefinite number of new macros over
> time, the simpler case API will never need to change.

No, it has to add an indefinite number of new enumerated values over 
time.  If we were iterating over these values there would be some code 
savings in doing this, but since all the calling checks are done 
explicitly there is none.

> Really, it is kindof sleasy salesmanship to say there are only 3 when you
> know there will be a huge number of additional macros coming down the pipes
> next - and such considerations are really important in interface design :-).

The only additional one I can imagine us ever needing is a hex check.

> You want to fold the backend into your three functions in a hardcoded
> way, i.e.that become part of the functional interface or API for all
> time. Really, you need to stop saying one thing, then doing exactly the
> opposite :-).

The enumeration is a part of the interface, not a part of the 
implementation.  Just because it is an enumeration does not make it 
magically more extensible or less hard-coded than a comparable set of 
macros.  The way we're using it there is no extra extensibility.

jason

? server/core.9972
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/30 17:09:30
@@ -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 (!topo_has_flag(TF_WRAPX)) {
+    nat_x = CLIP(0, nat_x, map.xsize - 1);
+  }
+  if (!topo_has_flag(TF_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/30 17:09:30
@@ -180,6 +180,17 @@
   struct map_position *start_positions;        /* allocated at runtime */
 };
 
+enum topo_flag {
+  /* Bit-values. */
+  TF_WRAPX = 1,
+  TF_WRAPY = 2,
+  TF_ISO = 4
+};
+
+#define CURRENT_TOPOLOGY (TF_WRAPX)
+
+#define topo_has_flag(flag) ((CURRENT_TOPOLOGY & (flag)) != 0)
+
 bool map_is_empty(void);
 void map_init(void);
 void map_allocate(void);