[Freeciv-Dev] Re: (PR#4381) some clean with get_map_tile(...) , get_map_terrain(...), get_map_special(...) ...

["Rafa³ Bursig" <bursig@xxxxxxxxx>]

Dnia 2003.06.10 19:29, Per I. Mathisen napisa³(a):
> 
> I like most of it (modulo some style issues), but not these:
> 
> -      map_set_special(x, y, S_FARMLAND);
> +      ptile->special |= S_FARMLAND;
> 
> We call map_set_special() for a reason, and that reason is code
> abstraction. This code is run very rarely (compared to other code) so
> optimizing them in this way is not a good idea.
> 
Yes I can agree with you but you don't lose code abstraction when add
#define tile_set_special(ptile, special)        ptile->special |= 
special

problem of reset_move_costs(...) came only with road and railroads and 
we can call this manualy in those cases when it is needed ( or check it 
in those cases )

this is still question of politic, current we recalculate the same tile 
many times.

> This is not an optimization:
> 
>  bool can_unit_change_homecity(struct unit *punit)
>  {
>    struct city *pcity=map_get_city(punit->x, punit->y);
> -  return pcity && pcity->owner==punit->owner;
> +  return pcity && pcity->owner==punit->owner && 
> pcity->id!=punit->homecity;
> 
ops... sorry this may be next patch becouse it change functionality.
but IMHO it is good change becouse you don't need allow change home 
city when unit come from that city ( you shouldn't have active menu 
options )

> PS Hopefully also you realize that the speed improvement from your
> patch will be quite insignificant. As opposed to the speed 
> improvement that
> would be gained from making map_get_tile() into a macro.
> 
Per... I know that in times of current/moderns CPU this type of coding 
can be replaced by good code
abstraction/maintability/cleanest/etc. but it is still question of 
philosophy. You must understan that as hardware engineer I always try 
save resources becouse you never know when will you need it.
When I made this patch I found many places (in many cases loops) where 
developer (or many developers) use both ptile and 
map_get_terrain/spcial/city of the same map position.

Our current x,y API is good and work exelent when we need only know 
about tile info one times in function. Those functions are only wrapers 
on map tile systems and IMHO are waste when we want this info many 
times in loop.

map_get_tile() is only wraper on other macro MAP_TILE(x , y), other API 
function use this macro directly. Changing map_get_tile() into macro 
without changing current code rather won't change many only reduce this 
function jup. When you connect it with my idea (replacing [x,y] API by 
ptile direclty) you get mouch better results.

I don't say that we should remove current x,y API only that we should 
use direct ptile as priority.
We can ...

rename current contains_special(enum tile_special_type set,
                      enum tile_special_type to_test_for) to
                     debug_contains_special(enum tile_special_type set,
                        enum tile_special_type to_test_for)
And

#ifdef DEBUG
#define contains_special(set, to_test_for) debug_contains_special(set, 
to_test_for)
#else
#define contains_special(set, to_test_for) ((set & to_test_for) == 
to_test_for)
#endif

then we add

#define tile_get_special(ptile)                 ptile->special
#define tile_set_special(ptile, special)        ptile->special |= 
special
#define tile_clear_special(ptile, special)      ptile->special &= 
~special

#define tile_has_special(ptile, special)         
contains_special(ptile->special, special)

#define tile_set_terrain(ptile, terrain)        ptile->terrain = terrain
#define tile_get_terrain(ptile)                 ptile->terrain

#define tile_set_continent(ptile, continent)    ptile->continent = 
continent
#define tile_get_continent(ptile)                       ptile->continent

#define tile_set_city(ptile, pcity)             ptile->city = pcity
#define tile_get_city(ptile)                    ptile->city

In this way we got nice code abstraction API based on tile system.

Rafal