[Freeciv-Dev] Re: Remove map_adjust_[xy] invocations fromserver (PR#1003)

[Jason Dorje Short <vze2zq63@xxxxxxxxxxx>]

"Ross W. Wetmore" wrote:
> 
> At 05:18 AM 01/10/12 -0400, Jason Dorje Short wrote:
> >Raimar Falke wrote:
> >[snip: uses of is_real_tile, normalize_map_pos, etc.]
> >
> >> I plan to propose something like this:
> >>  - there is a CHECK_MAP_POS(x,y) == assert(is_normal_map_pos(x,y));
> >>  - every "assert(is_real_tile(x,y));normalize_map_pos(x,y)" is
> >>  replaced by CHECK_MAP_POS
> >>  - there is an extra check (if(!is_normal_map_pos(x,y)){y=x/0;})
> >>  introduced in map_inx
> 
> As far as I am aware, is_normal_map_pos() is not the same thing as
> is_real_tile(). It tests for normalized and not realness which are
> different concepts that you really need to keep straight.

Correct.  What Raimar is proposing is not a substitution of a macro for
identical code, but rather "fixing" the code to always pass normal
coordinates.

> Some of Jason's cases are just a check for normalized, and that is what
> his macro is designed to do. This does not mean Raimar should break the
> code everywhere by replacing the is_real_tile(), normalize_map_pos()
> constructs used elsewhere.

"Break the code everywhere" is a bit strong.  There are a few places
where it may be necessary to fix existing code to meet the above
requirements, but for the most part everything is already taken care of.

> So if you add this macro do it with the right check, and document so
> carefully that idiots cannot misunderstand what it does.

Yes.

> If you need a second macro, then make it and document it in the same
> way.

I do not believe a second macro is needed, but Raimar thinks we may want
different levels of checking (and so use different macros). 
Additionally there is the problem with map_inx and other places where a
macro cannot be used (AFAICT).

> [...]
> >1.  I do not see the logic of forcing a crash when not in debugging
> >mode.  Presumably people playing the game in this stage would appreciate
> >it if we did everything possible to avoid crashes.  For the "extra
> >check", why not have something like this:
> >
> >if(!is_normal_map_pos(x, y)) {
> >  assert(0);
> >  freelog(...);
> >  nearest_real_pos(&x, &y);
> >}
> 
> Please add BIG BAD COMMENTS to explain that this is a GROSS HACK to keep
> playing in the face of a SEVERE PROGRAMMING ERROR that is not yet fixed.

Yes.

> Note it may seriously damage gameplay, as any illegal game coordinate
> transformation is liable to do.

I don't think it can damage gameplay any more than abort() would. :-)

In general, though, I believe CHECK_MAP_POS should do the following (and
I think Raimar is in agreement):

- If the coordinates are normal, do nothing.
- If the coordinates are non-normal and real, assert(0) and run
normalize_map_pos().
- If the coordinates are non-real, either (1) abort() or (2) assert(0)
and run nearest_real_pos().  This has not been decided on yet.

> But I think you want to use normalize_map_pos() here to safely continue
> because if you were concerned about coordinates that might be unreal
> and hence need some gross hackery, then you should have tested with
> is_real_tile().
> 
> Seriously, are you checking for unreal or just unnormalized coordinates?

Unnormalized.  However it would be desired IMO for the game to not crash
just because unnormalized coordinates happen to be passed (unless you've
compiled with DEBUG, in which case the assert() should take care of it)
(especially since in the conversion it may take a while to make sure
we've gotten everything cleaned up).  In the case of unreal coordinates
things could go either way (again IMO) since nearest_real_pos() may
still give you bad coordinates that ruins the game (and it's a more
serious programming error than the first scenario).

> [...]
> >2.  None of this is a reason not to go ahead with this patch.  This
> >replaces one set of imperfect code with another set, but at least the
>           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ make this clear
> >new set uses the desired code constructs (i.e. normalize_map_pos instead
> >of map_adjust_x()).

"imperfect" because both patches continue to unnecessarily normalize
coordinates on every function call.

> >Either way, I'm willing to come up with a patch that makes the checks we
> >decide on (although fixing the code to use them everywhere will take
> >more work).  Let me know what you want.
> >
> >jason
> 
> Actually, I think you got the right fix a few emails later. It used
> normalize_map_pos() in the test and nearest_real_pos() in the fail clause
> which is at least consistent.
> 
> But this should still be documenteed as a GROS HACK as above :-).

It's so localized, I'm not sure I'd call it a "gros hack".  What we are
talking about is a function or macro called check_map_pos(x, y) that
should be called before you assume (x, y) is normal; this is all the
user of the code really needs to know.  The only use of this is in
debugging: it can both help to find bugs (with an assert() when problems
are found) and keep bugs from being fatal (by using normalize_map_pos()
and possibly nearest_real_pos()).  The internals of the function/macro
are really a separate issue.  The goal is to avoid the overhead (both in
execution time and code complexity) associated with calling
normalize_map_pos() each time, but this goal is a little ways off at
first since check_map_pos() will have a lot of overhead as well (in
execution time, that is).

jason