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

[Jason Dorje Short <vze2zq63@xxxxxxxxxxx>]

"Ross W. Wetmore" wrote:
> 
> At 10:54 AM 01/10/24 +0200, Raimar Falke wrote:
> >On Tue, Oct 23, 2001 at 09:24:00PM -0400, Ross W. Wetmore wrote:
> >> Whole_map_iterate() currently *guarantees* the map positions are all
> >> not only real, but normalized ... no checks are needed until you start
> >> to add new map types. I think Raimar just got carried away in his
> >> enthusiasm to bog doen the code with checks here.
> >
> >All this stuff [Jason] is thinking about is preparation work for other
> >map types. This will reduce the size of the final patch which adds
> >other topologies.

BTW: not only does it reduce it in size, but makes it readable and makes
it possible to validate each step much more easily.  To put it another
way, if we have this much disagreement over each individual change we'd
never get through a full one-patch change.

> As far as I am aware, no other topology currently under consideration
> needs these checks. The loop constraints are sufficient. So save this
> baggage until it is needed, though leaving it in the MACRO in comments
> is a trivial way to keep everyone happy.

Like I said in my other post, the loops will only work if you impose
specific boundaries, and even then they have other limitations.

> >> >My understanding is that whole_map_iterate also doesn't guarantee
> >> >anything about the order in which the coordinates are traversed.  The
> >> >stuff in server/savegame.c certainly needs such a guarantee.
> >>
> >> This is true, but it is easily remedied by making a fixed order part of
> >> the interface, or providing versions that do have fixed orders.
> >
> >It may be nice to have macros for this but I see for example no point
> >in providing a macro which is used two times in savegame.c. So we have
> >to do some kind of requirement analysis.
> 
> Adding one or two extra macros like this is really not that significant
> in terms of code bloat. Adding a comment to point one at the location
> that really needs this special flavour is also a useful documentation
> technique for someone that decides they need the same flavour and then
> rolls their own when they see that the general macro is insufficient.

What kind of macro are you talking about?

I've mentioned the idea of having x_map_iterate/y_map_iterate macros,
for instance of the form

#define y_map_iterate(y_itr)
  for (y_itr=0; y_itr<map.ysize; y_itr++)

#define yx_map_iterate(y, x_itr)
  for (x_itr=0; x_itr<map.xsize; x_itr++)
    if (regular_map_pos_is_normal(x_itr, y) {

It would be easy to change these loops to use a more efficient form
later.  Raimar didn't think these provided any extra clarity to the
code, though, and they make it harder to have non-normal handling.

The savegame stuff also needs explicit checks for non-normal positions
(well "needs" is a bit strong.  It uses them for simplicity and human
readability); the debugging code in game.c needs them as well.  This
would have problems using a macro like this - it could be done, but the
current code is more readable.

As Raimar says, loops like this are pretty rare.  Our time would be
better spent IMO figuring out whether the debugging methods in game.c
need to be kept than arguing what macro they should use to iterate over
the map.

> To put it bluntly, it is not the current use, but the future abuse that
> you are fixing :-).

Using a macro would make it easier for future changes to see "the right
way" to change the code, yes.  However, they don't make the code any
more readable (see above) and in many cases may make it less (if you
need explicit checks for non-normal positions).

> Anyway, I presume that savegame really only needs it for backwards
> compatibility with older saved games. Any games written and read back
> in with any alternate ordering should still work, right?

Yes, if they use the same ordering.

The problem is whole_map_iterate doesn't say anything about the
ordering.  Right now it's ordered y,x for cache efficiency.  One could
imagine that someone would change the ordering later and not think
anything of it, breaking compatibility without even realizing that was a
possibility.  For code like this (and also the debugging code that needs
to be human-readable) a fixed loop is more guaranteed to handle things
correctly.  That doesn't mean I'm opposed to macros, though, I just
think there are some places that shouldn't use them.

jason