[Geany-Devel] On document pointer recycling

[Nick Treleaven]

On 26/10/2013 00:22, Lex Trotman wrote:
> On 26 October 2013 00:01, Nick Treleaven <nick.treleaven at btinternet.com>wrote:
>
>> On 24/10/2013 10:24, Lex Trotman wrote:
>>
>>> Okay, but you still agree that doc->is_valid should be removed eventually?
>>>>> That's a step forward:)
>>>>
>>>
>>> Of course I agree.  So its not a terribly big step:)
>>>
>>
>> I only skimmed the discussion, but how can we remove that?
>>
>
> Given your experience it would be good if you could read the discussion and
> give your perspective on what the previous posters have discussed regarding
> handling doc pointers properly.

I read the discussion.

In summary, I think there are 3 issues:
* forgetting to check is_valid when iterating documents
* freeing memory
* code that uses is_valid to check if a document still exists, which is 
bad practice

I think freeing memory is a non-issue, I think you (Lex) agree with me 
on that. It's not a memory leak, it may be reused later.

The first point needs to be handled before the last one (assuming each 
are worth fixing). I think we can:

* remove GeanyDocument::index and fix any code that uses it
* review all code indexing documents_array and fix any code retaining an 
index (I expect that is rarer than code using GeanyDocument::index).
* change documents_array to only contain valid documents, i.e. on close 
we swap the last valid document pointer with the pointer being removed, 
and decrement the length by 1.
* use a free list for invalid documents

The last point is important to ensure no memory corruption bugs are 
introduced. I think it is harder to find the code that retains document 
pointers (and there is some), so I recommend we don't free document 
memory. At least, we would need a better reason to do so IMO than just 
memory usage, which is insignificant.

Now, once we've done that it's no longer needed to check is_valid when 
iterating documents. Possibly we could remove is_valid, but I think that 
is less important to do. Note that if we do all the steps above, we 
don't need to break the API, only the ABI. I can't guarantee that bugs 
won't be introduced due to these changes, but it seems fairly reliable.