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

[Jason Dorje Short <jshort@xxxxxxxxxxxxx>]

Gaute B Strokkenes wrote:
> 
> On Thu, 26 Jul 2001, jshort@xxxxxxxxxxxxx wrote:
> > Gaute B Strokkenes wrote:
> >
> >> The reason this is dangerous is that normaliz_map_pos() will
> >> sometimes change the value of x.  If we compile with NDEBUG, this
> >> does not happen, and a crash may occur.
> >>
> >> I think we should changes these uses to is_real_tile() instead, and
> >> use normalize_map_pos() explicitly if we rely on the value of x
> >> being normalised afterwards.
> >
> > Ouch!  That's bad!
> >
> > The assertion is there no doubt because the tile is supposed to be
> > normalized to begin with - however, as you point normalize_map_pos
> > doesn't check that for the X value.  Unless there's time to debug
> > things more, the safest solution for the release would be to move
> > normalize_map_pos out of the assertion.  After the release, this can
> > be replaced by an assert(is_real_tile(...)) call.
> [snip]
> > (BTW, I think is_normal_map_pos would be a better name than
> > is_real_tile.)
> 
> You're confused.  You are mixing the concepts of validity (i.e. "Do a
> given set of coordinates refer to an existent tile?" or "y >= 0 && y <
> map.ysize") and canonicalisation ("Are these the officially blessed
> coordinates for a given tile?" or "x == map_adjust_x(x)").  Think
> about it.

I was confused, but not quite the way you describe :-)

I recognize the difference between a "normal" and a valid tile/map pos,
but I am confused as to exactly what the code
assert(normalize_map_pos(...)) is _intended_ to do.  I think it may
differ from place to place.  I didn't realize until looking at the code
that is_real_tile checks for validity, not normalcy.

What the assertion does now is normalize the X value, while dying if the
Y value isn't real (only if debugging, of course).  My code dies if both
coordinates aren't normal, which isn't the same thing but I assumed
(perhaps incorrectly) is what was intended.  I assumed that the writer
actually did intend to put the function call into the assertion but
didn't realize that normalize_map_pos doesn't have the same return value
as is_normal_map_pos would.  If the writer did intend to have the X
value normalized, then they made a much bigger mistake by placing the
function call within the assertion!

Most likely different invocations of the assertion had different
intents, so they'll require different substitutions.  Your substitution
(assert that the value is valid, then normalize it) would work in all
cases but would result in spurious normalizations.  After taking a few
minutes to do the substitutions my way, it appears that most but not all
of the assertions are intended to be the way I assumed.  Most of the
gtk-client ones are not, however.

I do believe is_normal_map_pos() is a good check that should be used.

jason