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

[Ben Webb <ben@xxxxxxxxxxxxxxxxxxxxxx>]

On Sat, 12 Jan 2002, Vasco Alexandre Da Silva Costa wrote:

> On Sat, 12 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.

        Well, it didn't reach my machine until 4:56am. Today. I also 
don't have an internet connection at home; however, this is not the issue.

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

        In my opinion, 1 run from each of 4 people is rather better than 
5 runs from one. (Particularly as there _were_ some bugs that I 
missed in rather more than 5 runs, reported on Freeciv-dev back in 
December.) Plus, there was ample opportunity in those nine months for 
Freeciv maintainers to disagree with my coding style, rather than making 
the changes and slipping the code into CVS without discussion.

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

        Well, perhaps, but it actually implements the functionality of 
get_relevant_effect_lists() more closely than get_effect_lists().

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

        The impr_effects_iterate macro requires this functionality, plus 
the ability to pass a NULL city pointer. This impacts at least the 
govchange, boost-research, and global-city patches, from memory. Of 
course, you may be going to write a separate get_all_effect_vectors() 
function (or similar) to do this.

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

        If you return a variable length array, you need to malloc it 
somewhere; otherwise you're just asking for trouble.

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

I don't think this is negligible.

        Ben
-- 
ben@xxxxxxxxxxxxxxxxxxxxxx           http://bellatrix.pcl.ox.ac.uk/~ben/
"You know my methods, Watson."
        - 'The Crooked Man', Sir Arthur Conan Doyle