[Freeciv-Dev] Re: remove nearest_real_pos uses (PR#1211)

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

At 01:01 PM 02/01/07 +0100, Raimar Falke wrote:
>On Mon, Jan 07, 2002 at 02:39:16AM -0800, jdorje@xxxxxxxxxxxxxxxxxxxxx wrote:
>> The attached patch removes two unnecessary (incorrect AFAICT) uses of 
>> nearest_real_pos.
>> 
>> 1.  In map_get_player_tile(), I've replaced the nearest_real_pos call 
>> with a CHECK_MAP_POS check (included in map_inx).  I haven't verified 
>> that all calls to the code are safe, but (1) the error message that is 
>> currently displayed for non-real tiles is never shown and (2) 
>> extensively running autogames has yet to turn up a failed assert.
>> 
>> Note that Ross's corecleanups return NULL in the case of a non-real map 
>> position, but this isn't desired IMO and isn't feasible at this time 
>> (since it would require converting all uses to check the return value, 
>> which they currently do not do).

Matter of style to some extent. Returning a NULL value allows the caller
to handle the error condition, as opposed to deciding that the one-true-way
is pre-decided by fiat. Of course the caller can decide to resolve the
error condition by indirecting through the NULL pointer which will 
accomplish much the same end as your assert would do but in a more 
optimized way :-).

Note that the code you removed would only have displayed an error for a
non-real tile. The corecleanups used normalize_map_pos() to make sure 
that the TRUE condition correctly normalized, and nearest_real_pos()
would have effectively done the same.

Thus if there were unnormal coordinates passed in you would never have 
seen it, so your assurance on this just evaporated. You *will* die now 
though if this is the case :-).

But if you turn off error asserting, you will pass back pointers into
random memory in this case.

>> 2.  In auto_settler_do_goto(), it has been simply replaced by 
>> CHECK_MAP_POS.  Here I've scanned all the calls to the function, and it 
>> looks like they should all pass Good coordinates.  Again there have been 
>> no failed assertions in my autogames.
>> 
>> Note that Ross's corecleanups handle this incorrectly, as they call 
>> normalize_map_pos on the coordinates and ignore the return value 
>> (verboten!  Ross, you should know better!).  This will result in 
>> unpredictible behavior if non-real coordinates are ever passed in; most 
>> likely (and hopefully) a failed CHECK_MAP_POS check later on when the 
>> coordinates are actually used.

Technically, checking for an unreal case should always be done as part
of any normalize_map_pos(). But since the interface to this function 
requires real coordinates in the corecleanups, in much the same way that 
you now make all function interfaces require normal ones, the callers 
understand that passing unreal coordinates is bad. 

Callers used to pass unnormal ones though, which is a soft condition that
can always be fixed. In this case normalize_map_pos() is guaranteed to 
fix things and doesn't need to be checked. 

There was an extensive test period for realness fixes and fair amount
of code scrutiny at one point before the corecleanups were distributed
publicly and there has been a much longer settling time for these
things there. 

Someday, maybe you will understand this all fully, and recognize when
trying to score points will backfire :-).

It would be nice though if we could just cleanup the code, rather than 
play games about who is better qualified to do it :-).

>> If this patch is applied, the only remaining nearest_real_pos use will 
>> be in map_pos_to_canvas_pos in mapview_common.  If one of these places 
>> does need to use nearest_real_pos (which seems inconceivable), a comment 
>> should be added explaining why.

If this function returns status, and get_canvas_xy propagates this to
its callers who then deal with the situation, this one can go.

This is the comment/reccommendation in the core_cleanup.

>Can't nearest_real_pos then be moved into mapview_common? So nobody
>can use this "dangerous tool". However this would spread topology
>knowledge (which is also bad). What about just ignoring clicks which
>are outside the map?

Better fix this case, then let it join map_adjust_y() and SAFE_MAPSTEP
in the bit bucket.

>       Raimar
>
>-- 
> email: rf13@xxxxxxxxxxxxxxxxx
>  Living on earth may be expensive, but it includes an annual free trip
>  around the sun.