[Freeciv-Dev] Re: PATCH: check_map_pos

[Gaute B Strokkenes <gs234@xxxxxxxxx>]

On Tue, 23 Oct 2001, vze2zq63@xxxxxxxxxxx wrote:
> Gaute B Strokkenes wrote:
>> 
>> On Tue, 23 Oct 2001, vze2zq63@xxxxxxxxxxx wrote:
>> > Gaute B Strokkenes wrote:
>> >>
>> >> Why would you want or need it to be an lvalue?  The function
>> >> you've given below is not an lvalue.
>> >>
>> >> #define map_inx(x,y) \
>> >>   (CHECK_POS((x),(y)), map.tiles + (x) + (y) * map.xsize)
>> >
>> > Sorry, I didn't mean an lvalue.  But the fact remains that if you
>> > make the below function into a standard macro it will not fit in
>> > map_inx.
>> 
>> #define CHECK_POS(x,y)                                     \
>>   (is_normal_map_pos(*x,*y)                                \
>>    || (freelog(...), normalize_map_pos(x, y))              \
>>    || (freelog(...), abort())) /* or nearest_real_pos() */
> 
> No.  freelog() cannot be used in this way.

Ah, I see.  Someone has (rather frivilously) wrapped the freelog macro
in a do { .. } while (0).  There is no reason to do it like that; the
easy solution would be to fix freelog().

> #define CHECK_MAP_POS(x, y)                             \
>   (is_normal_map_pos(...)                               \
>    || (assert(0), normalize_map_pos(...))               \
>    || (abort(), 1))
> 
> This would work as desired, but without the freelog messages (which
> are of very limited use anyway).

Yup.

>> That's pointless.  Your proposal combines the worst features of the
>> different models.  On the one hand we have to spend time and effort
>> to make sure that all coordinates that we create are normalised,
>> and on the other hand we have to make sure that we accept
>> unnormalised coordinates (possibly with a squeeky warning message)
>> everywhere.  It would be better to stick to the system that we have
>> now.
> 
> You're missing the third goal: to avoid crashing if we've compiled
> with NDEBUG.

My point is that spending lots human and CPU time on making sure that
we always create normalised coordinates is a pointless waste of timeq
unless we are going to take advantage of that effort.  The current
situation is not particularly buggy, just slow.

The best way to avoid a bug causing a crash is to remove the bug.  Off
all the possible sorts of bugs that could cause a crash, these are
some of the more unlikely ones.  We have invested some effort in
coming up with ways to manipulate coordinates in a safe manner, and
the sort of sloppiness which would result in such a bug is easily
identified.

>> I think you're being very unclear.  What, _precisely_, does "you're
>> about to assume the coordinates are normal" mean?
> 
> A difficult question.
> 
> Most functions do not assume coordinates are normal, but a very few
> (like same_pos, IS_BORDER_MAP_POS, and map_inx) do.  This is where
> the checks need to go.
> 
> More precisely, I think it happens when you're about to use the
> specific values of the coordinates for comparison or calculation, as
> opposed to when you just pass them off to some other function or use
> them for relative calculations.  This is a pretty weak explanation,
> but it's the best I can do.

It is much simpler and reliable to simply decree that all coordinates
should be created normalised and real.  That way you do not have to
play guessing games over what someone might be doing with your
coordinates.

-- 
Big Gaute                               http://www.srcf.ucam.org/~gs234/
A dwarf is passing out somewhere in Detroit!