On 26/10/2013 00:22, Lex Trotman wrote:
On 26 October 2013 00:01, Nick Treleaven nick.treleaven@btinternet.comwrote:
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.