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

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

Jason Short wrote:
> 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:

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.

The interface you wanted is just one function map_hasFlag(). There is
no need to split it into a separate function for every Flag that one
can come up with from now to eternity :-).


>    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 :-).

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.

> 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

This code is not the interface ... it is a particular implementation
fragment. Implementation can always change if it is properly hidden
from the interface.

One should *never* have to know how the backend works, i.e. any one of the
there different coding styles above should be usable in a backend with no
change to the way you write the application code. To write app code, you
just need to read the interface description. Someday maybe you will learn
to look at things this way, maybe?

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

To go back to your original example, please keep any current implementation
details like the fact that there are currently only 2 wrapping dimensions
labelled X and Y out of the API, or there is something like an ISO flag.
These are nitty gritty implementation things that should never be in the
hardcoded function definition for a generic flag test.

>    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.

Each of those operations returns a very generic value. The values have well
defined meanings that you can learn about be reading the header description
of the API. The meanings of these won't change over time even if one adds
new map types or flags.

Unintuitive is mostly a matter of desire rather than complexity in such
cases, methinks. Some people just don't like short function names as they
never contain enough characters to adequately pin down the details.

> 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 :-).

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

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 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:

That is right. The map types should be enumerated, and not built up as
combinations of internal details like flag bits. The real reason for
this is that not all bits are independent, and many bit combinations
would not make sense. So the bit form is not completley generic way to
view things, rather just a part of the implementation.

There is a little implementation code that takes any maptype and a flag
value question and figures out how to use it, rather than exposing the
details of the flags implementation through the API and thus hardcoding
them for alltime.

>    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?

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 :-).

This is what you avoid by moving the characters in the function name
into the argument list as enum/variables.

one generic flag test function:

     map_hasFlag(TOPO_FLAG_WRAPX)
     map_hasFlag(TOPO_FLAG_WRAPY)
     map_hasFlag(TOPO_FLAG_ISO)
     map_hasFlag(TOPO_FLAG_HEX)
     map_hasFlag(<TOPO_FLAG_FUTUREn>)

       vs

an indefinite number of API tests since the flags are always extensible

     map_hasWRAPX()
     map_hasWRAPY()
     map_hasWRAPTheta()
     map_hasWRAPPhi()
     map_hasISO()
     map_hasHEX()
     map_isEarth()
     map_hasFilterEllipse()
       ...

> jason

Cheers
RossW
=====