[Freeciv-Dev] Re: Map coordinate cleanups.

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Thu, Aug 16, 2001 at 10:15:47PM +0200, Gaute B Strokkenes wrote:
> On Thu, 16 Aug 2001, hawk@xxxxxxxxxxxxxxxxxxxxxxx wrote:
> > On Thu, Aug 16, 2001 at 02:15:07AM +0200, Gaute B Strokkenes wrote:
> >> 
> >> +    int x1 = pcity->x + x - 2;
> >> +    int y1 = pcity->y + y - 2;
> > 
> > Can a function created which does this transformation? It is used
> > all over the place. This would be the reverse of
> > common/city.h:get_citymap_xy().
> 
> I think that's a good idea; a macro which loops over the neighbourhood
> of a city, skipping unreal tiles and delivering both local city
> coordinates and normalised global coordinates would be even better.
> 
> [Best of all, we may wish to get rid of the idea of local city
> coordinates alltogether and replace them with just a number.  As the
> above sentence shows, the whole idea of local city map coordinates is
> rather confusing anyway.]

Ack. In another patch hopefully.

> >> -  for (tt = T_FIRST; tt < T_COUNT; tt++) {
> >> -    if (0 == strcmp (tile_types[tt].terrain_name, name)) {
> >> +  for (tt = T_FIRST; tt < T_COUNT; tt++)
> >> +    if (!strcmp (tile_types[tt].terrain_name, name))
> >>        break;
> >> -    }
> >> -  }
> >> +
> > 
> > Maybe it is time to decide which rule freeciv should follow. I would
> > vote for the extra {}s.
> 
> I vote against, obviously.  But I don't feel very strongly about it.
> I suspect that there is already a rule for this: the style guide
> mandates K&R style, so someone who cares can go and look it up and
> then tell the rest of us what it is.

I didn't found something on the net. And I'm not old enough to have a
K&R book.

> >> -  for (inx = 0;
> >> -       inx < 
> >> sizeof(tile_special_type_names)/sizeof(tile_special_type_names[0]);
> >> -       inx++) {
> >> -    if (type & 0x1) {
> >> -      return tile_special_type_names[inx];
> >> -    }
> >> +  for (i = 0; i < NUM_SPECIAL_NAMES; i++) {
> >> +    if (type & 0x1)
> >> +      return tile_special_type_names[i];
> >> +
> >>      type >>= 1;
> >>    }
> > 
> > Could this be rewritten to use ffs(3). There is no loop needed.
> 
> I'll leave that to someone else, if desired.  Until and unless that
> someone does that, my change is still good.

Ack.

> >> +#if 1
> >>  #define map_inx(x,y) \
> >>    ((x)+(y)*map.xsize)
> >> +#endif
> > 
> > Mhh.
> 
> Yeah, I know what you mean.  I'll remove them.  I thought I had done
> so already...
> 
> >> +/* FIXME: These are particularly useless, but used everywhere.  */
> > 
> > These methods provide abstraction.
> 
> They don't.  get_tile_type() takes a terrain identifies and returns a
> struct describing that terrain, without any error checking.  An array
> look-up is a perfectly acceptable, commonplace idiom to go from a FOO
> identifier to FOO_TYPE descriptor struct.  Moreover, if you use the
> array look-up method the reader will know without any further ado that
> there is checking on arguments etc.  (one of the things I really
> dislike about Freeciv is the way that there seems to be no rhyme or
> reason to which of the map_get_* and associated functions check and/or
> repair their arguements.)

The abstraction is about knowning the underlying global variable or
not. If all access to this variable is through methods or macros the
variable can be removed from the global scope. I consider this a good
thing.

Besides this I also dislike the mixed use of ids and pointers. There
is no way to get from the name of the method the types of the
arguments. I like get_tile_type() because it is a clear id-to-pointer
method. There should be such a method for every object with a
consisting name. Currently there is find_unit_by_id(),
get_tile_type(), find_city_by_id(), get_government() and so on. Also
each struct should have a field "id" and not something like
"player_no". I would like to get the opinions of others to the
id-vs-pointer topic.

> In essence, using function-like syntax instead of an array look-up is
> not abstraction.  To be `abstraction' there has to be something
> non-trivial to abstract.
> 
> Another thing you might wish to do is to do 
> 
>   $ grep -Ir get_tile_type * | wc -l

66

> and
> 
>   $ grep -Ir tile_types * | wc -l

136

> in a clean source tree.  Direct access to tile_types outnumber
> get_tile_type() calls by two to one.

Yes the current situation is inconsistent. However the question is:
which construct is the goal?

> >> +#if 1
> >> +#define MAP(x,y) (map.tiles + (IS_PROP_TILE((x),(y)) \
> >> +                               ? ((x) + map.xsize * (y)) \
> >> +                         : (abort(), 0)))
> >> +#else
> >> +#define MAP(x,y) (map.tiles + (x) + map.xsize * (y))
> >> +#endif
> > 
> > Mhh.
> 
> Please be more specific.  When I'm done only one of these and
> map_get_tile() will remain.  In the meantime, I find them very useful.

Ok.

> >> +#define MAPSTEP(x,y,x1,y1,dir)     \
> > 
> > I like this one.
> 
> Thanks.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 "Only one human captain has ever survived battle with the Minbari
  fleet. He is behind me. You are in front of me. If you value your 
  lives, be somewhere else."
    -- Ambassador Delenn, "Severed Dreams," Babylon 5