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

[Petr Baudis <pasky@xxxxxxxxxxx>]

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.

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() ?

> > > Note that I commited the patch already.
> > 
> > It appears to me that the development is getting more chaotic now..
> > (compared to situation in November/December) It looks there's not even
> > clear consensus between the maintainers what're the current goals and what
> > should we do now; like vasco commiting recently the patches in order to
> > make g++ happy, or like you saying that cleanups are low-priority and we
> > should focus on finishing features for 1.12.1 now (before about a month)
> > (this apparently changed mysteriously and without any notice i cought; and
> > I believe your previous opinion was much more correct).
> 
> I agree. Please take a look at the sound patch. If this is in let us speak
> about the last two (game startup, unit activation). I also agree that I found
> a new tool and I do currently play heavily with it. But this does benefits
> freeciv IMHO.

I'll try to have a look at it soon.

> > IMHO we just need to review the patches and have some time for that (like 2
> > days at least - after all, we don't hurry anywhere, do we?).  I had a
> > little different opinion before two months, but now it looks like this
> > doesn't work well; like the patches which got commited too soon after
> > posted for review or which didn't posted for review at all (like the
> > pointers stuff or all [or almost all] vasco's patches). It seems we have
> > two or three (raimar, vasco and mike [he is at least interested in my job
> > ;]) active maintainers now, so what about enforcing the policy that
> > maintainer should never commit own patches?  I think we are getting into
> > trouble now with this 'cleanups', changing too much things too fast.
> 
> In general I agree. However this my not work in practice. For example I have
> sent an email one week ago asking the other maintainers to review and commit
> the sound patch. Nothing happend. (Note maintainers that this isn't an attack
> at you. I know that time and interrest is limited).

But we should probably always at least try...

> Bottom line: no problem waiting for two days.

Fine.

-- 

                                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/