[Freeciv-Dev] Re: Unsafe assertions. (PR#864)

[Gaute B Strokkenes <gs234@xxxxxxxxx>]

On Fri, 27 Jul 2001, xyzzy@xxxxxxxxxxxxx wrote:
> On Fri, 27 Jul 2001, Thue Janus Kristensen wrote:
>> On Fri, Jul 27, 2001 at 11:24:58AM -0700, Trent Piepho wrote:
>> > On Fri, 27 Jul 2001, Thue Janus Kristensen wrote:
>> > I think Jason's way was a lot better.  You have weakened all the
>> > assertions by only checking is_real instead of checking for
>> > is_normal.  You also normalize every time, when the author might
>> > very well have intended to assert that normalization was NOT
>> > necessary.
>> 
>> Note that I did not weaken the assertions. normalize_map_pos()
>> returns TRUE even if the tile is not normalized.
> 
> But when the author wrote the assertion, did they mean to assert
> that it was a real tile, or that it was a normalized position?

In the absence of any further evidence, it seems reasonable to assume
that the original author actually bothered to have a look at what
normalize_map_pos() before they decided to use it.  So I think we may
safely assume the former.

> If the former, then why didn't they use is_real_tile to being with?

Probably because normalize_map_pos() is designed to be a one-stop,
all-shop[1] solution to the problem of wobbly map coordinates: make
sure it's a real tile AND canonicalise the coordinates; all with a
minimum of fuss.

> Probably because they thought that normalize_map_pos() would return
> false if the coordinates were not normalized.
> 
> But who wrote that mess anyway?  Maybe they can shed some light on
> what the hell they were thinking.

I don't think it's a mess and no, I didn't write it.  This part of the
code seems to have been written with the Golden Rule in mind: be
liberal in what you accept, and conservative in what you emit.  In
other words: accept non-normalised coordinates without barfing, and
try to make sure that any coordinates you do produce (or pass to other
functions) are normalised.  I think that's a perfectly reasonable
approach to take.

Another reasonable approach would be to always normalise coordinates
when you create them, and make it a hard error to attempt to use
non-normalised coordinates.  This would avoid the overhead of
normalising everything twice.

Some time ago Thue wrote a set of consistency checking functions for
the server with which we have squashed a number of bugs.  I've printed
out map.c and map.h and started marking the bits that I don't
like/feel like changing in green, and I intend to present a general
cleanup patch for those files soon.  One of the things I would like to
do is to have a single macro

  #define MAP(x,y) (map.tiles + (x) + (y) *  map.xsize)

with which to acces the map.  Then depending on whether we are
compiling with consistency checking on or off, we can replace it with
a version that also makes sure that (x,y) is real and normalised.
That would help us to most, if not all, places where unnormalised
coordinates are created.

  [1] I'm not sure I got this Americanism right, but you know what I
  mean.

-- 
Big Gaute                               http://www.srcf.ucam.org/~gs234/
..  over in west Philadelphia a puppy is vomiting..