[Freeciv-Dev] Re: PATCH: check_map_pos

[Jason Dorje Short <vze2zq63@xxxxxxxxxxx>]

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.

#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).

> >> 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.

You're missing the third goal: to avoid crashing if we've compiled with
NDEBUG.

As far as effeciency goes, we only have two choices.  Either we check
is_normal_map_pos with DEBUG and NDEBUG (as above), or only with DEBUG (as
below).  How we handle things if this check fails is insignificant.

> > 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?

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.

> 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.

Again, a secondary objective is to prevent bugs from being damaging during a
release.  This is balanced against having the release perform efficiently.

> 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.

Indeed.

So I again say: the alternative to the macro above is this one:

#define CHECK_MAP_POS(x,y) assert(is_normal_map_pos(*x,*y))

We can easily switch between them, so there's no need to spend time arguing
about what we use so long as it fits this interface.

jason