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

Not just bad practice, but wrong, it does not guarantee its the same document.  Its unlikely, but possible, if the holder of the pointer does not correctly handle document close.

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.

Yes agree, the memory is not technically "lost" and the amount does not grow over time, although its re-use depends on the users usage pattern, which I think was what Thomas was concerned about.  But if you had enough memory to open that many documents with all the scintilla overhead, then keeping the GeanyDocument struct isn't material.

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

If Geany or any plugin keeps a pointer (or an index) to a document they have to properly handle document close, even with the current implementation, to ensure that their pointer (or index) points to the same document, and if they handle that properly they won't have an invalid pointer or index.  That requirement is independent of how the document list is implemented, and it should remove all need to use is_valid outside iteration.

Then the issue of forgetting during iteration can be fixed.

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

Yes, the memory is not an issue, but the real problem (not handling close) needs to be fixed wherever it occurs, and changing the implementation won't help.  In fact, as I said in the PS that started this thread, the current implementation at least ensures that the pointer/index will always reference something that is a GeanyDocument object, if the document structs were de-allocated in the normal way the pointer could point to anything.

Although the chances of the wrong document being referenced because of a retained pointer are slim, any fault that can happen, will happen (and at the worst moment according to Murphy).

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.

An API break doesn't matter really if we are going to break the ABI, that requires all plugins to be recompiled anyway.  So that says we should only do this when something forces an ABI break anyway.



Devel mailing list