[Freeciv-Dev] Re: remove nearest_real_pos uses (PR#1211)
[Top] [All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index] [Thread Index]
Ross W. Wetmore wrote:
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 :-).
True, but (1) none of the code does any checking now and (2) it's safer
to rely on an assert than on the caller to use the value in a way that
will cause a segfault. Since there's no legitimate reason to make a
NULL request, there's no advantage to allowing such.
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.
But I have added CHECK_MAP_POS in its place, which may do that if desired.
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 :-).
Yes, my statement was unclear. The autogames have not died, but if
there is a bad reference we will now be able to catch it easily.
But if you turn off error asserting, you will pass back pointers into
random memory in this case.
Again, it depends on the implementation of CHECK_MAP_POS. Assuming all
calls are correct no check is needed and we will be fine.
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.
This is the reverse of the argument you made up above! You will end up
with invalid coordinates that may not be caught later, pointers into
random memory, etc., etc. The only difference is that it depends on a
hard rather than a soft condition, but once it happens that won't be
much consolation...
What I'm saying is that the return value of normalize_map_pos should
_never_ be ignored. If you assume the coordinates you're given are
real, you should check this with an
is_real = normalize_map_pos(&x, &y);
assert(is_real);
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.
So is CHECK_MAP_POS, if we implement it that way. But as long as the
soft condition is met we will be safe.
Again, if we implemented CHECK_MAP_POS as:
#ifdef DEBUG
# define CHECK_MAP_POS(x, y) assert(is_normal_map_pos(x, y))
#else
# define CHECK_MAP_POS(x, y)
if (!normalize_map_pos(&x, &y)) {
freelog(...);
nearest_real_pos(&x, &y);
}
#endif
Then we'll have exactly the same effective code, plus the additional
assertion to help us catch bugs, and it will be uniform throughout.
This was considered when CHECK_MAP_POS was first implemented, but it
requires the arguments to all be valid & operands which wasn't the case
at that time. Predictably, the places where this wasn't the case
quickly failed the check and have been removed, so it should be safe to
write it like this now. It's a choice between speed and safety - you
can't have both.
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.
In that case the assertion definitely shouldn't fail - all the more
reason to put it in :-).
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 :-).
I'm just explaining why my implementation differs from the corecleanups.
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.
Unfortunately no, since the only way the callers can deal with it is to
call nearest_real_pos. We can let it go if we agree to give up the
functionality it provides.
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.
map_adjust_y is almost gone, and SAFE_MAPSTEP can be implemented locally
(i.e. moved to tilespec.c). Only nearest_real_pos provides
functionality that isn't availibe outside the topology backend.
jason
|
|
