[Freeciv-Dev] Re: [Patch] Add BOOL_VAL around ANDs

[Jason Short <vze2zq63@xxxxxxxxxxx>]

Raimar Falke wrote:
> On Tue, Feb 12, 2002 at 05:13:21PM +0100, Petr Baudis wrote:
>
> > Dear diary, on Tue, Feb 12, 2002 at 04:35:41PM CET, I got a letter, where
> > Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx> told me, that...
> >
> > > On Tue, Feb 12, 2002 at 03:21:56PM +0100, Petr Baudis wrote:
> > Glancing at it, it basically appears ok, however:
> >
> > @@ -1190,10 +1190,19 @@
> > ..snip..
> >
> > /***************************************************************
> > + Returns TRUE iff the given special is found in the given set.
> > +***************************************************************/
> > +int contains_special(enum tile_special_type set,
> > +                    enum tile_special_type to_test_for)
> > +{
> > +  return (set & to_test_for) == to_test_for;
> > +}
> > +
> > +/***************************************************************
> > ...
> > ***************************************************************/
> > void map_set_terrain(int x, int y, enum tile_terrain_type ter)
> > @@ -1208,7 +1217,7 @@
> > {
> >   MAP_TILE(x, y)->special |= spe;
> >
> > -  if (BOOL_VAL(spe & (S_ROAD | S_RAILROAD)))
> > +  if (contains_special(spe, (S_ROAD | S_RAILROAD)))
> >     reset_move_costs(x, y);
> > }
> >
> > I just can't help myself, but when only S_(RAIL)ROAD will be set, the
> > contains_special() will return false, won't it? We either need to make the test
> > in contains_special() as !!(set & to_test_for), or (MUCH more cleaner) make the
> > test bellow (contains_special(spe, S_ROAD) || contains_special(spe,
> > S_RAILROAD)). Note that there are multiple occurences of this bug in the patch.
> Good spotting. I also vote for the second solution.
The second solution is good (makes things more readible), but the first 
is essential.  Otherwise anyone who misuses it in the future will get 
very odd behavior.
Eeither !!(set & to_test)for), BOOL_VAL(set & to_test_for), or

  assert((set & to_test_for) == to_test_for
         || (set & to_test_for) == 0);

(or something similar to that effect).

jason