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

["Jason Short" <jdorje@xxxxxxxxxxxxxxxxxxxxx>]

rwetmore@xxxxxxxxxxxx wrote:
> 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.

In the corecleanups you have

   enum MAP_TYPE {
     MAP_TYPE_FLAT    = 0,  /* Note: not a WRAPTYPE */
     MAP_TYPE_WRAPX   = 1,
     MAP_TYPE_WRAPY   = 2,
     MAP_TYPE_TORUS   = 3,
     MAP_TYPE_ISO     = 4,  /* map is stored in isometric form */
     MAP_TYPE_ISEARTH = 8,  /* map is earth == special type    */
     MAP_TYPE_LAST    = 16
   };

   #define is_maptype(TYPE)        (map_type == (TYPE))
   #define get_maptype(TYPE)       (map_type & (TYPE))
   #define has_mapwrap(WRAPTYPE)   (get_maptype(WRAPTYPE) == (WRAPTYPE))

which I guess is what you consider "data-driven".  The problem is that 
this leads to all sorts of unreadable constructs:

   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:

   }

which are perfectly logical but make for an awful and bug-prone 
interface - you HAVE to know how the backend works in order to use the 
interface properly.  As another example you can have

   if (get_maptype(MAP_TYPE_TORUS)) {

   }

versus

   if (has_mapwrap(MAP_TYPE_TORUS)) {

   }

versus

   if (is_maptype(MAP_TYPE_TORUS)) {

   }

which are quite different in very unintuitive ways.


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.


The basic problem with your system is that MAP_TYPE_TORUS (or any | 
combination of flags) is a map type and cannot intuitively be used as a 
flag value.  But because the interface does not make this distinction 
there is a problem.  To keep the interface simple the callers should 
only use three forms:

   has_mapwrap(MAP_TYPE_WRAPX)
   has_mapwrap(MAP_TYPE_WRAPY)
   has_mapwrap(MAP_TYPE_ISO)

which return boolean values.  But if you do that, why not make the 
complete separation between interface and backend and just have three 
separate macros?

jason