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

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

I actually like something like this better than nearest_real_pos(). I am
assuming when it is expressed correctly it effectively returns the original
unstepped coordinates. More importantly, it should clearly document that 
this is what it does as part of the interface.

The case where one steps in a loop until move points are exhausted, and
hits an unreal tile along the way (causing a serious slowdown in game
response :-) is at least indirectly covered by this warning/documentation
then.

At 05:23 AM 02/01/07 -0500, Jason Short wrote:
>jdorje@xxxxxxxxxxxxxxxxxxxxx wrote:
>
>
>> I do think SAFE_MAPSTEP is wrongly implemented.  It needn't use 
>> nearest_real_pos but should instead do
>>   #define SAFE_MAPSTEP(x, y)
>>   {
>>     int _x = x, _y = y;
>>     if (!normalize_map_pos(&x, &y)) {
>>       x = _x;
>>       y = _y;
>>     }
>>   }
>
>Oops - I forgot I snuck this in in the most recent tilespec fix.  So 
>scratch that idea :-).
>
>jason

But if you look closely ...

In fact what you have done here is one of the many local variants of
normalize_map_pos() that are going to spring up because you didn't 
make the condition we all say is desired, i.e. the one used here, part
of the interface, and thus force people to implement this privately all 
over the codebase.

It is better to just do it. Think of it as a code reduction measure, 
i.e. of all the duplicate code you are saving by moving it into the
implementation from just outside :-).

If someone really wants arbitrary values returned, and the void_tile
sentiments revive to carry the day to give this a new lease on life, 
then it is possible to expose the underlying internal interface in 
normalize_map_pos(), i.e. below the is_real check and let them 
experience the Chinese curse of "may you live in interesting times".

This way at least code that uses the primary interface won't suffer.
And you will have to go slightly out of your way to do the stupid
thing. It is always wise to make the defaults safe, and not the 
"special" case :-).

Cheers,
RossW
=====