[Freeciv-Dev] Re: PATCH: map iteration (PR#1018)

[Jason Dorje Short <vze2zq63@xxxxxxxxxxx>]

"Ross W. Wetmore" wrote:
> 
> At 05:17 AM 01/10/30 -0800, jdorje@xxxxxxxxxxxxxxxxxxxxx wrote:
> >jdorje@xxxxxxxxxxxxxxxxxxxxx wrote:
> >>
> >> There are a lot of places in the code that have loops like
> >>
> >>   for (y=0; y<map.ysize; y++) {
> >>     for (x=0; x<map.xsize; x++) {
> >>       ...
> >>     }
> >>     ...
> >>   }
> >>
> >> These loops will generally not work under a general topology, since they
> >> are intended to loop over only normal, real positions and for many
> >> topologies that will not happen.
> >>
> >> The easy solution is to do an is_normal_map_pos check, as in:
> >>
> >>   for (y=0; y<map.ysize; y++) {
> >>     for (x=0; x<map.xsize; x++) {
> >>       if (is_normal_map_pos(x, y)) {
> >>         ...
> >>       } else {
> >>         <possibly some extra handling>
> >>       }
> >>     }
> >>     ...
> >>   }
> 
> These are whole_map_iterate() loops. They should be replaced with
> whole_map_iterate().

So I have said.  I also said they couldn't be replaced (AFAICT), because
whole_map_iterate does not (and should not) guarantee the order of
positions found, and also makes it very difficult to handle wrap-around
cases like the second "..." in the above example.  Your corecleanups do
not change any of these to whole_map_iterate (unless I've missed
something), so I am still in the dark as to how to convert them.

> You need block_iterate() to get the ones that are a subset of
> whole_map_iterate().

block_iterate will not work any more than whole_map_iterate for these
loops.  There may be other loops that I have missed that can use it, but
since they probably use normalize_map_pos anyway they won't be a
problem.

> >> The cleanest example of this is in whole_map_iterate, where the
> >> is_normal_map_pos check can just be added directly.
> 
>  ... when it is needed :-)
> 
> But you might find better ways to handle the implementation of
> whole_map than doing large amounts of looping and filtering out the
> few cases you really need. So the change may never need to be added.

Yes, but I haven't.  If you have found a better way, feel free to share
it.  Please provide code.

> >So, now back to this patch...
> >
> >Questions:
> >
> >1.  Ross, do you see the necessity of these changes yet?
> 
> I have replaced whole_map_iterate in more places than you have found :-)

Unless I'm looking at an old or incomplete version of the corecleanups,
I don't see where.

> I have still not needed to fix manufactured regular problems :-) :-)

You do not consider the cases where normal!=regular, so you have not
even considered the problems that it presents.

> >2.  Are there any more loops that can be replaced by iterator macros?
> >Such replacement is IMO the best solution possible.  But, I don't see
> >how to convert any more of them.
> 
> block_iterate

Please explain.

> >3.  Is the debugging function print_landarea_map() in game.c (part of
> >the LAND_AREA_DEBUG group) necessary?  If no consensus can be reached, I
> >suppose it must be left in.
> 
> Someone removed a lot of these from mapgen at one time. I put new ones
> back when I need to work in the code. There's probably a lesson in there
> somewhere ...

I take it that's a vote for keeping (unless you mean that the lesson is
more code comments are necessary).  That's good enough for me.

> >4.  Is regular_map_pos_is_normal good, or should we stick to
> >is_normal_map_pos?  Again, regular_map_pos_is_normal provides identical
> >functionality but assumes the map position it is given is "regular",
> >i.e. within (0..map.xsize-1,0..map.ysize-1).  This makes it much faster.
> 
> I'm not much of a fan of regular. You need to demonstrate it a bit
> more to show any real value. If it never does anything, then dubious
> dregs like this are just noise.

"Regular" is what is there now, in just about every single loop. 
Therefore, the onus is on you to demonstrate that it should be removed
and to explain how to do so.  The easy solution (and probably the most
efficient one as well) is just to filter on what is normal.

> Another maxim that comes to mind is premature optimization is the root
> of all evil. I might be best to wait until you have demonstrated there
> is a problem before optimizing. That shouldn't take too long, though.

My original presentation of this patch explained the slowness of using
is_normal_map_pos in every iteration.  It is not much, but it is easily
avoidable.  Raimar therefore suggested regular_map_pos_is_normal (though
we took some time to give up on finding a better name for it).

jason