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

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

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).

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

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
  "brand memory are for windows users that think their stability
   problems come from the memory"
    -- bomek in #freeciv