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

[Petr Baudis <pasky@xxxxxxxxxxx>]

Dear diary, on Wed, Feb 13, 2002 at 09:59:52AM CET, I got a letter,
where Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx> told me, that...
> On Tue, Feb 12, 2002 at 05:50:53PM -0500, Jason Short wrote:
> > 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).

Assert is ok, to prevent possible misuse. But we shouldn't _permit_ the misuse
at all.

> Ok. The question is still: 1, 2 or 3 methods?

I would vote for 3 methods obviously :).

-- 

                                Petr "Pasky" Baudis

* UN*X programmer && admin         * IPv6 guy (XS26 co-coordinator)
* elinks maintainer                * FreeCiv AI hacker
* IRCnet operator
.
I love deadlines.
I love the whooshing sound they make as they fly by.
-- Douglas Adams.
.
Public PGP key && geekcode && homepage: http://pasky.ji.cz/~pasky/