[Freeciv-Dev] Re: (PR#2521) general effects framework

["Mike Kaufman via RT" <rt@xxxxxxxxxxxxxx>]

On Sun, Dec 08, 2002 at 06:50:13PM -0800, Vasco Alexandre Da Silva Costa via RT 
wrote:
> 
> On Sun, 8 Dec 2002, Mike Kaufman via RT wrote:
> 
> > Here is the first draft of the first step toward general-improvements.
> > This is the initial framework.
> 
> And it is great stuff indeed. I like it a lot! ;)

thanks.
> 
> I think it would be nicer if you added an EFR_ALL #define which just ORed
> all the flags or something. It would be a more intuitive name since that
> is a bitvector argument.

I agree. will do.

I probably should define an EFT_ALL as well, though it'll simply be
#define EFT_ALL EFT_LAST

and now that I think about it, B_* are going the way of the dodo, so B_LAST
should probably go too...

> 
> Another personal nit is I think function arguments 2-5 look odd in that
> function. I suggest you make it thus:
> 
> range.type   = EFR_ALL;
> range.player = pplayer;
> range.city   = pcity;
> range.impr   = walls_impr_id;
> effect_iterator_init(&iter, &range, EFT_SPY_RESISTANT);
> 
> However MicroSoftie that may seem :)
> 
> Another idea is that you could add a macro wrapper for cases like the one
> above to ease coding. Thus:
> 
> range.type   = EFR_ALL;
> range.player = pplayer;
> range.city   = pcity;
> range.impr   = walls_impr_id;
> 
> effect_iterate(iter, &range, EFT_SPY_RESISTANT) {
>   failure_chance += iter->defn->ammount;
> } effect_iterate_end;

I'm not a big fan of this. range must be independent of the particular
arguments player, city, id. besides which this requires an additional
struct. you could assign these to the iter struct, but then it would seem
strange to have to call init on it (which you would have to do). 

in either case, you would have to know the internals of a struct which I
don't like all that much. But you're right, I too feel that what I've got 
ain't an ideal solution to the problem.

What I have provided are a couple of iterate macros in effects.h:

#define city_effects_iterate(type, pcity, _eff) \
{ \
  struct effect_iter _iter; \
  struct effect *(_eff); \
  struct player *_plr = (pcity) ? city_owner(pcity) : NULL; \
  effect_iterator_init(&_iter, EFR_WORLD|EFR_PLAYER|EFR_ISLAND|EFR_CITY, \
                       _plr, (pcity), B_LAST, (type)); \
  while( ((_eff) = effect_iterator_get_next_active(&_iter)) ) {                

#define city_effects_iterate_end \
  } \
}

(which when looking at it has a bug)

and 

#define nonlocal_effects_iterate(type, _player, _eff) \
{ \
  struct effect_iter _iter; \
  struct effect *(_eff); \
  effect_iterator_init(&_iter, EFR_WORLD|EFR_PLAYER|EFR_ISLAND|EFR_CITY, \
                       (_player), NULL, B_LAST, (type)); \
  while( ((_eff) = effect_iterator_get_next_active(&_iter)) ) {                

#define nonlocal_effects_iterate_end \
  } \
}

the city effects iterate will get used a lot, and this is the method that I
would prefer to ease coding. Obviously though, there are all sorts of cases
where this isn't going to be good enough. If we can think of other classes
of cases, we can add more iterator macros.

-mike