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

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

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:
> > > Dear diary, on Tue, Feb 12, 2002 at 02:03:40PM CET, I got a letter, where
> > > Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx> told me, that...
> > > > > As for the patch, you might as well make a function
> > > > 
> > > > > bool contains_special(int tspecials, enum tile_special_type special)
> > > > 
> > > > This would be the one with the lowest level and will always possible.
> > > 
> > > Agreed.
> > > 
> > > > > or even
> > > > 
> > > > > bool tile_has_special(struct *tile ptile, enum tile_special_type
> > > > > special)
> > > 
> > > This would be the real encapsulation being done. Though....
> > > 
> > > > Sometimes we have no ptile.
> > > 
> > > This should occur mostly only in the part of freeciv where we mess with 
> > > the
> > > map itself, thus not breaking the encapsulation. We can just call
> > > contains_special(ptile->special, special) from tile_has_special(ptile,
> > > special).
> > 
> > See my patch.
> 
> 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.

> But this patch doesn't introduce the tile_has_special() - I think when we're
> doing this, we should do it properly. Most times when we are comparing 
> directly
> to the special mask, we get it from the map_get_special(x, y). Can't we get 
> the
> ptile for that coords instead and pass it to the tile_has_special() instead of
> contains_special() ?

This would be three extra function which are similar. 

We have
77  map_has_special
55  contains_special(...->special,...)
137 other contains_specials

These number are rough based on grep.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 "That's fundamental game play!  My main enemy is *ALWAYS* fighting 
  a 4-front war.  I make sure of it!"
    -- Tony Stuckey, freeciv-dev