[Freeciv-Dev] Re: (PR#3450) square_iterate and unreal positions

["rwetmore@xxxxxxxxxxxx" <rwetmore@xxxxxxxxxxxx>]

Jason Short wrote:
> Ross Wetmore wrote:
>>Raimar Falke wrote:
[...]
> When we first added the checks to the iterator macros, it was pointed 
> out (and nobody disagreed with) the fact that none of them were actually 
> needed.  They were added anyway (by Raimar and myself), 

I have always objected and you have always gone ahead anyway :-).

 > as (I thought) a
> temporary measure to catch a few extra buggy cases 

All the temporary measures seem to have become permanent, even the
buggy ones. There should be a lesson for you in blindly proceeding
along such paths and ignoring the warnings.

> that might not be 
> caught in other places.  As far as I know, they have not caught a single 
> one.
> 
> So I in favor of phasing them out as needed.

Good, please do so, it is needed :-)

In fact, you have caught things that were perfectly valid as has been
shown with these square_iterate() examples. Introducing bugs that cause
core dumps in valid codepaths should be considered a bad approach to
solving problems - always.

> As a side note, I am strongly against adding new iterators with cryptic 
> names.  And for me, the "checked" modifier is as cryptic as it gets. 
> Currently we have only one of these, and it has a very different meaning 
> for "checked" than you are using.

The new meaning of "checked" is the problem, not the existing one :-).

In olden times, there were no checks that produce core dumps at every
memory access point or global operation. There were just a scattering
of checks at the places where new coordinates were generated or the
start of code blocks that other than for their inputs, were correctly
coded. The difference between unreal (terminal error) and unnormalized
(always a locally fixable problem) were also correctly managed, so the
current flavour of dumping core in cases of fixable problems was never
seen.

> (I can't really tell what the "checked" in city_map_checked_iterate 

At least you recognize and admit it, two important first steps ...
and hopefully then will refrain from doing anything to it until you
do figure it out.

> means; I think it just means it normalizes the positions before 
> returning them.  For that matter, I can't really tell what most of the 
> city.h iterators do just by looking at the names...even after reading 
> the descriptions it is difficult to match name with description.)

Iterators don't normalize positions. The function for this is
normalize_map_pos().

Iterators generate a pattern of positions and sort them into normalized
and unreal sets in their if and else clauses.

Pure city iterators run over all citymap coordinates. There is no validity
checkIng other than the coordinates lie in a city grid pattern. The latter
is only needed if one uses an inferior implementation like looping over
a larger set and selecting those that match the city grid pattern. The
iterator inteface has no concept of checking though - there is a single set
of outputs.

The checked city iterators actually check the city coordinates against
corresponding map positions and return outputs that are sorted into
normalized and unreal map positions. To do this they also need additional
information to map a city to a map location at at least one point (not
required for the pure citymap case).

Pure city iterators are used for things like initializing citymaps or in
GUI displays where you actually want to step over all grid positions
regardless of map validity or Raimar's religious sensibilities.

The checked city iterators should be used in all cases where you are
updating the citymap from map data, i.e. when you need to guarantee
the validity of map accesses and need map coordinates as well as city
coordinates.

One can actually use pure city iterators for lookup and summary ops
on data that has been cached in the citymap, i.e. without checking
for map validity if one has correctly cached the pertinent data. You
will find cases that do this and others that use the checked subset
in such situations. From a performance standpoint, it may be that the
checking even if one does not use the map coordinates is more or less
expensive than the summarizing step.

> jason

Cheers,
RossW
=====