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

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

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

Clearly you need to learn to distinguish the relationship between caller and
called code and when changes to the latter percolate up to the former :-).

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

You are so FUDful when you have nothing more useful in a counter argument 
... :-)

Read on, or meditate on the caller/called code idea above ... then feel
your backend again and remeber who did it :-).

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

Only locally where they are used the new code. The general macro *never*
needs to be updated in the header which is where the interface is defined,
while it is possible to add new data entries to the enum in only the new
local code if you use appropriate coding styles.

This is standard object oriented thinking - keeping interface and
implementation separate.

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

I guess this all depends on your definition of simple to paraphrase one
rather infamous politician.

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

The definition of the interface should not be ambiguous. You then code it
according to the definition. There are two concepts here and bad coding or
documentation is not a counter argument, as anyone can code badly even if
the architecture allows things to be done cleanly and well.

One is "has_only_flag" the other "has_any_of_flags". As has been said before
by using the implementation coding of the bits and ORing them together in
the argument, you confuse the interface and the implementation. You need to
define the interface to take a single enum value which under the covers may
be ORed bits, but which clearly means any of the set of, or only this one
flag. If someone changes the implementation to replace the bits with an
array of values or something down the road, the inteface and application
code should not need to change.

You objected and removed the FLAG and set parts to the enums from one of
my updates and changed things back to exposing the bits. I suspected this
was because you didn't understand the problems and ambiguities at that
time. I have asked you to reread those emails several times.

Your code flavour has this same ambiguity problem though, so it is not a
counter argument to the form.

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

I think you actually got it right in the above statement. But I'll wait to
see if this is really true, or just wishful thinking. I suspecct you got
it right and then inverted things again in the final step, though.

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

One minor coding argument that doesn't cover the general case is not an
argument for anything. Even a hundred such examples gets blown away by one
real general one, though running through a 100 may make a lot more noise.
So you wasted bandwidth beyond providing the general case.

>>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 stopped reading the postings again ...

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

Enumeration is a coding style. There are other styles that do the job
better, and ways to override enumerations that make them extensible if
you want to go this route.

In future cases, you may want to make these data variables that are read in
from rulesets, and not hardcoded programmatic enums.

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

good ... I think you might have understood both the concept and the need for
interface documentation.

> +  TF_WRAPX = 1,
> +  TF_WRAPY = 2,
> +  TF_ISO = 4
> +};
> +
> +#define CURRENT_TOPOLOGY (TF_WRAPX)
> +
> +#define topo_has_flag(flag) ((CURRENT_TOPOLOGY & (flag)) != 0)

You could emphasize this here, and fix up the enum to provide the set forms
explicitly so a bit flavour becomes hidden implementation and not interface.
Thus no one will ever be tempted to OR the bits themselves as arguments in
local code that will need to be tracked down and fixed when a new set form
of implementation is added.

And be aware, there is already a need for sets of sets that means bits are
not the only elements needed in the general case.

> +
>  bool map_is_empty(void);
>  void map_init(void);
>  void map_allocate(void);

Cheers,
RossW
=====