[Freeciv-Dev] Re: list cleaning

[Raimar Falke <i-freeciv-lists@xxxxxxxxxxxxx>]

On Tue, Jan 27, 2004 at 12:53:38PM +0000, Per I. Mathisen wrote:
> On Tue, 27 Jan 2004, Raimar Falke wrote:
> > > > YOU SHALL NOT REMOVE ITEMS FROM A LIST OVER WHICH YOU ITERATE
> ...
> > > Nested iterators are used all over the place, and not just in one place.
> > > You have eg an interator that calls procedures that have their own
> > > iterators that may or may not decide to remove units in the list.
> ...
> > Note the above rule is already in effect.
> 
> We have plenty iterators of the kind
> 
>       unit_list_iterate_safe
>               delete unit (x)
>       unit_list_iterate_safe_end
> 
> already. This is "remove items from a list over which you iterate".

Ok. Then I change this to

  YOU SHALL NOT REMOVE ITEMS FROM A LIST OVER WHICH YOU ITERATE UNLESS
  YOU USE A SAFE VARIANT OF THE ITERATION

> > You make it sound like the current server is buggy as hell. I don't see
> > evidence for this.
> 
> No, I believe we've fixed these errors for now. However, the framework is
> not solid, and further changes in the server are likely to expose further
> defects in the code and/or create new bugs due to the unclear nesting.

> > This is also a large problem I have with your current lazy-delete patch.
> > It just doesn't feel safe. With an early destruction you can for example
> > catch access to this now dead memory with valgrind. With the lazy
> > approach you can't.
> 
> What access? Any data pointed to by the list should be deleted the same
> moment that the list node is marked as 'deleted'. To not do so is a
> separate error.
> 
> If you both forget to delete the memory used by the data pointed to by the
> list node that is deleted, _and_ then try to access it, then, yes,
> valgrind will not give you a warning. But this is the case now as well.
> 
> (We could in the future expand the lazy delete patch to give warnings when
> this is not done by checking the pointer for a NULL value before deleting
> the pointer node - this is optional and would require some code cleaning.
> However, such errors are very rare, and valgrind will report them as
> memory leaks anyway.)
> 
> So any access to deleted nodes (other than iteration, of course, which is
> the entire point of the patch), will give valgrind warnings as usual.

You are correct we have the data (struct unit * for example) and the
node. I'm speaking here about the node data.

> The 'feel safe' argument I am going to blithely ignore.

> > > If we have, eg, a nations_init(), we should also have a nations_done(),
> > > just for compleness, so that the day someone needs to add a cleanup
> > > operation in the latter, he does not have to second-guess the original
> > > author on where it needs to be put.
> >
> > I agree but the only connection I see with the current issue is that
> > you want to place the lazy-delete function in the *_free() function?!
> 
> We will need a lazy-delete call in the genlist_free() procedure, of
> course, in addition to calling it every turn done.

Not if we do the
delete-lazy-nodes-after-ending-the-(nested)-iteration thing.

> Your previous objection to the patch seemed to be that adding such
> *_free() calls had too much code impact.

You spoke about modules i.e. pairs of .h and .c files. IIRC the patch
added the free calls to a lot of more places.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 Q:  Do you know what the death rate around here is?
 A:  One per person.