[Freeciv-Dev] Re: list cleaning

["Per I. Mathisen" <per@xxxxxxxxxxx>]

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

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

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.

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

  - Per