[Freeciv-Dev] Re: [PATCH] Map cleanups (PR#1208)

[jdorje@xxxxxxxxxxxxxxxxxxxxx]

Ross W. Wetmore wrote:

> Mapclean_1:
>
> This is the first update to mapclean which fixes a number of issues
> from the past week discussions.
>
> 1)  style fix missing space after "," 
>
> 2)  update comments to refer to Pythagorean and Manhattan 
>
> 3)  change MAP_TILE to map_get_tile and make it a global macro
>
> 4)  move RANGE_CHECK and base wrap macro to shared.h
>
> 5)  remove changes to map_adjust_[xy]
>
> 6)  rework void_tile references to be more clear
>
> 7)  defer the is_normal_map_pos() until the debate is resolved
On a brief inspection:

- Your variables for macro names are very unreadable.  X versus PX? 
Does "P" stand for map?
- The double-definitions is a pretty ugly hack; I'm not a big fan.

- Either WRAP or the documentation of WRAP is wrong.  It needs a + 
(lower) on the end of it.
- WRAP and WRAP_0 correctly name their range variable as "range", but 
RANGE_CHECK and RANGE_CHECK0 call it "upper".  Anyone who reads this 
will be very confused to find that "upper" fails the check.  Call it 
"range" instead.
- map_to_index really needs a pos in there somewhere.  It should either 
be map_pos_to_index or map_to_index_pos.  I prefer the former.
jason