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

["Ross W. Wetmore" <rwetmore@xxxxxxxxxxxx>]

At 02:43 AM 02/01/14 -0800, jdorje@xxxxxxxxxxxxxxxxxxxxx wrote:
>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.

They haven't changed since the earlier submission. "P" is for pointer
and is a standard in some circles,

>- Either WRAP or the documentation of WRAP is wrong.  It needs a + 
>(lower) on the end of it.

Yes, good eyes - bad me.

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

No, it isn't a range, it is an upper. The documentation is clear.

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

No it doesn't. It returns an "index" not an "index_pos". The function
is *not* the same as the various <blort>_to_<blah>_pos(), as it does
not return status or take a reference argument for return position(s).
The leading pos has been debated and (wisely) dropped from all these 
functions, so this is consistent here.

Thus its signature is different and should remain so.

"Pos"es need to mean something or they shouldn't be put in "somewhere"
just to make things look nice :-).

>jason