[Freeciv-Dev] Re: CVS's civserver core dumps

[Vasco Alexandre Da Silva Costa <vasc@xxxxxxxxxxxxxx>]

On Sat, 12 Jan 2002, Ben Webb wrote:
> On Sat, 12 Jan 2002, Vasco Alexandre Da Silva Costa wrote:
> > On Fri, 11 Jan 2002, Ben Webb wrote:
>       Correct; however, the reason the function crashes is that it's 
> passed an invalid player pointer. Since update_all_effects() gets that 
> pointer from a bog-standard players_iterate, it's most likely that you've 
> introduced a memory corruption bug in some other function. The original 

Yes. And i've already mentioned i probably had a memory corruption bug
somewhere. Yesterday.

> impr-gen patch didn't reveal such a bug in over nine months of use in 
> FreecivAC.

True. But how many people tested that? 4? I didn't step on a bug in my
code on the 5 runs i did either.

>       My suspicions rest on get_effect_vectors() in 
> common/improvement.c, which I have several severe problems with. (It seems 
> to be my original get_effect_lists() function, with several bugs added.)

Yes it is based on get_effect_lists(). Notice the striking difference in
the function names. I did base the patch on your code you know? :-)
And i admited that function was buggy yesterday, don't need you to tell
me again.

> 1. You no longer pass a player pointer; thus there is no way to exclude 
>    Player- and Island- range effects any more. (I don't pass a player 
>    pointer just to avoid a call to city_owner(), you know.) This breaks 
>    impr-gen badly.

I didn't see any place that needed that functionality.

> 2. If there are multiple effects at a given range, you return the relevant 
>    geff_vector multiple times, which is rather wasteful.

The attached patch fixes this (it will be commited ASAP).

> 3. You return two variable length arrays, except that the first one is 
>    always of length 1 (and you implicitly assume this when calling the 
>    function). This seems rather bizarre.

I made it that way in case its needed for future expansion.

> 4. The vectors are returned in the order in which the effects specify 
>    them, rather than in order of increasing range. This breaks impr-gen 
>    badly.

The attached patch fixes this.

> 5. When you call the function, you pass in a geffs array of length 4 (this 
>    is a seemingly hard-coded constant - what was wrong with EFR_LAST?) 
>    Actually though, the returned geffs array is of dimension (number of 
>    effects). This is a segfault just waiting to happen.

I am going to set them all to EFR_LAST in the future.

>       Frankly, I don't quite understand why the original 
> get_effect_lists() function wasn't used, which included none of these 
> bugs. The patch has been available for testing since April, yet a greatly 
> modified patch was committed to CVS without any public testing at all. Is 
> it too much to ask that maintainers post their proposed changes to patches 
> on freeciv-dev, if they're going to make more than simple "style" changes?

Well, the function isn't that different. And the changes were required
because i split the event lists in two types instead of using struct athing.
I didn't think the changes i made were that relevant because they didn't
change the mechanics of your patch in a significant way (bar the bugs i
may have introduced).

The next patch in your series, effect-iterator, will most likely have
fairly significant changes from my part that will impact the other patches
and i will post that to freeciv-dev. Of course that will happen after all
the bugs in the patches i commited get squashed first.

---
Vasco Alexandre da Silva Costa @ Instituto Superior Tecnico, Lisboa

gev.patch
Description: Text document