[Freeciv-Dev] Re: [PATCH] check_map_pos change (PR#1031)

[Jason Dorje Short <vze2zq63@xxxxxxxxxxx>]

Gaute B Strokkenes wrote:
> 
> On Mon, 29 Oct 2001, vze2zq63@xxxxxxxxxxx wrote:
> 
> >> > I'm strongly opposed to the just-one-check-in-map-inx method of
> >> > doing this.  If there is a bug, we will never find it.
> 
> Why does this follow?

Without the assertion, it will be very difficult to catch bugs.  With
more assertions, it will be easier to catch bugs.

> >> > @@ -1128,6 +1125,7 @@
> >> >  ***************************************************************/
> >> >  int is_tiles_adjacent(int x0, int y0, int x1, int y1)
> >> >  {
> >> > +  check_map_pos(&x1, &y1);
> >>
> >> Why not check x0,y0?
> >
> > (x0, y0) need not be normal since the iterator macro right below it
> > will take care of things.
> 
> >> >  #define IS_BORDER_MAP_POS(x, y)              \
> >> > -  ((y) == 0 || (x) == 0 ||                   \
> >> > +  (check_map_pos(&x, &y),                    \
> >> > +   (y) == 0 || (x) == 0 ||                   \
> >> >     (y) == map.ysize-1 || (x) == map.xsize-1)
> 
> I see you have changed IS_BORDER_MAP_POS() and map_inx() so that they
> have to take lvalues as arguments.  What's the point?  As pointed out
> elsewhere, you can not reliably repair this sort of damage in the
> arbitrary topologies that you are proposing.

You can repair this damage in almost all topologies that people would
normally use.

But either way is fine with me; I'm tired of arguing over trivialities.

> > The iterate macros themselves work fine with non-normal coordinates:
> > they just add on offsets and then normalize.  Only in this check is
> > it specifically required that a position be normal (otherwise
> > IS_BORDER_MAP_POS will always return 0).  I'd prefer to leave the
> > check_map_pos near to where the actual assumption is being made.
> 
> As has been mentioned a couple of times already, the idea is to find
> bugs.  We are aiming for a model in which all functions should produce
> / expect normalised coordinates unless there is a very good reason why
> not.  So if we pass unnormalised coordinates to a funtion / macro,
> that's a bug; and we want to know about it.

If the idea is to find bugs, then check_map_pos should be placed
everywhere normalize_map_pos is now.  Obviously there are other
priorities as well.

> If we were to take the approach that you seem to be advocating, which
> is that it's ok to pass unnormalised coordinates to a function if you
> know that it won't matter, then we would have to carefully check the
> code everytime we make a change to make sure that it still will not
> matter, or to see if we need to add a check (and also check all the
> callers of that function or macro).  That will not make it any easier
> to debug Freeciv.

Ideally, we'd put the check at a place it would be called exactly once
for each set of coordinates.  But this isn't realistic.  Where, then,
should the checks go?  Clearly map_inx is the easiest place, but some
must go elsewhere (IMO).  In theory all the iterate macros should use
IS_BORDER_MAP_POS, so putting one there would make sense too (I'd rather
change the existing macros to all use IS_BORDER_POS than put a check
into each macro).

I have advocated putting them everywhere for now, and then drop them
later as desired.  But, I won't press my point.  Raimar wanted some
checks removed, so I removed them.  If you think a different set of
removals would give better results, that's great - you have a better
understanding of code flow than I do.


Your patch looks fine to me (although I haven't inspected it all that
closely).  My only question is what the advantage of

+#ifndef NDEBUG
+#define CHECK_POS(x,y) \
+  (assert(is_normal_map_pos((x),(y))), TRUE)
+#else
+#define CHECK_POS(x,y) TRUE
+#endif

over just

#define CHECK_POS(x, y) assert(is_normal_map_pos((x),(y))

is?  Not that this really matters; the implementation of the check is a
small issue compared to its usage.

jason