[Freeciv-Dev] Re: PATCH: check_map_pos

[Gaute B Strokkenes <gs234@xxxxxxxxx>]

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() */

>> I do not think that we need this sort of bells and whistles.  If
>> you take the approach that each piece of code should be as
>> forgiving as possible, then that is basically what we have now.
>> When you are debugging, a core dump is much more useful than a log
>> message since you can walk around the stack frame, inspect
>> variables etc. etc.  Note that with the above function even if you
>> get a log message you have absolutely no way of knowing _where_ the
>> problem occurred, only that it _has_ occured.  That makes its
>> usefulness very limited.
> 
> Sorry again :-)
> 
> You've haven't followed the full conversation, but the idea is that
> we assert(0) in all the problem cases within check_map_pos.  That
> way we get the core dump if we've compiled with debugging, and get
> corrected coordinates if we've compiled without.  The freelog's are
> still useful for non-debugging.

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.

>>  * GCC does not generate code of equal quality when you use inline
>>    functions instead of macros.  I have posted profiling results
>>    that show this before.  Another way to see this is to inspect
>>    the generated assembly directly.
> 
> Yes, I have seen this myself.  However the difference is small.

That's not true at all.  The profiles I posted about a week ago make
it very plain that the difference is significant.

> Checks are not needed everywhere; if a function just passes
> coordinates off to another function/MACRO it is not necessary to do
> them.  A function/MACRO that manipulates coordinates or uses them
> directly (like map_inx or adjc_dir_iterate, for instance) should
> check the coordinates before acting with them.  In other words,
> check_map_pos need only be called if you're about to assume the
> coordinates are normal; this will mostly occur in map_inx but not
> exclusively.  I believe we are all agreed on this?

I think you're being very unclear.  What, _precisely_, does "you're
about to assume the coordinates are normal" mean?

The object of the exercise is to detect bugs, the bug in this case
being coordinates which are accidentally not normalised/real.  So the
proper place to put it is wherever we think we might be able to detect
a bug.  Putting it inside map_inx() is just a convenience so we do not
have to litter the code with checks everywhere.

You really have to consider how likely such bugs are to occur, and
much trouble it is worth going to in order to detect them.

-- 
Big Gaute                               http://www.srcf.ucam.org/~gs234/
I'm RELIGIOUS!!  I love a man with a HAIRPIECE!!
 Equip me with MISSILES!!