[Freeciv-Dev] Re: [PATCH] map_adjust_* patch

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

At 07:51 PM 01/09/02 +0200, Gaute B Strokkenes wrote:
>On Sat, 1 Sep 2001, hawk@xxxxxxxxxxxxxxxxxxxxxxx wrote:
>>
>> I still like the idea of a is_border_position better than to do code
>> knowledge about the topologies into the iter macros
>
>I think that coding knowledge about topologies into the (4 or so)
>iteration macros is perfectly fine.  Since there are so few of them
>and they are so critial to performance it makes sense to give them the
>knowledge they need to walk the map in an efficient way.  

Agreed. There may be a small number of other cases as well.

>I don't quite see how you would apply is_border_pos() in most of the
>code; 

You don't. The only value of is_border_pos() or is_border2_pos() is to
return a TRUE/FALSE value that is faster to test than the leading
tests of normalize_map_pos(), and yet covers all the cases you would
need when iterating over a well-defined block of coordinates. If you 
do an is_border_tile() everytime, you likely just end up with the same
test sequence for a FALSE and double the test time for a TRUE.

>I think it is in most cases more natural to do
>
>  x += DIR_DX[dir]
>  y += DIR_DY[dir]
>
>  if (IS_SANE_POS(x,y) || normalize_map_pos(&x, &y)) {
>    /* Frobnicate. */
>  }
>
>in the general case.    

You just doubled up the tests, as above. Look at the lead-in code for 
normalize_map_pos(). Whenever, you preface something with "it is
natural", one can be almost certain that it is not well thought through :-).

>you need to be very careful about calling normalize_map_pos() on
>coordinates that are also controlling variables in a loop.  I haven't
>looked too closely, but I'm not satisfied that this is not the case
>where Jason change outwared_iterate().

Good point. 

But I didn't find any problems like this in the changes I have which
are a superset of Jason's. And I don't think he did anything radically
new.

And I did check. Then checked again, after the discussion about GUI updates
which are okay, because the normalized values are always separately defined
and not the ones handed to the GUI draw code or built into the loops.

At least the x-coordinates are usually safe, the y-coordinate handling
will need to be fixed for generality.

The other aspect that saves you, is that normalize_map_pos() doesn't 
update unreal values, so if y goes off the end, a loop would just spin
until it resets or runs out.

>> (see Gautes point from above). AFAI see normalize_map_pos can/will
>> become more complicated if other topologies are introduced. There is
>> no nice way to code normalize_map_pos later as a macro.
>
>So let's make normalize_map_pos() an inline function, that way we can
>both have our cake and eat it too.  

Make it anything you want. Since the current complexity is easily 
handled as a macro, you can have your cake with icing ...

1)  Macros work everywhere, so you don't break Freeciv, ever.
2)  Macros will never be any worse than current inline functions, and
    are likely better optimized.
3)  The API is identical, so you can replace anything with anything
    when the conditions of some part of the implementation change, like
    the code becomes too unweildy to do in a macro.

But, it doesn't *really* matter ... except for the religious aspects.

>-- 
>Big Gaute                               http://www.srcf.ucam.org/~gs234/
>Yow!  It's a hole all the way to downtown Burbank!

Cheers,
RossW