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

["Ross W. Wetmore" <rwetmore@xxxxxxxxxxxx>]

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.

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.

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

If you need a second macro, then make it and document it in the same
way.
 
[...]
>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.

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

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?

[...]
>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()).
>
>
>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 :-). 

Cheers,
RossW
=====