[Freeciv-Dev] Re: Map coordinate cleanups.

[Gaute B Strokkenes <gs234@xxxxxxxxx>]

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

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

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

>> +#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.)

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

and

  $ grep -Ir tile_types * | wc -l

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

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

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

Thanks.

-- 
Big Gaute                               http://www.srcf.ucam.org/~gs234/
Didn't KIERKEGAARD wear out his TIRES in VIENNA during a SNOWSTORM of
 FREUD's unpaid DENTAL BILLS?