On 13-10-26 06:22 AM, Lex Trotman wrote:
On 26 October 2013 23:17, Nick Treleaven nick.treleaven@btinternet.comwrote:
[snip]
- 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).
If the GeanyDocument were a GObject (or we added hand-rolled reference counting), the document would never get freed from under the code that uses them's backs. It's a different way of thinking of it, but it's like std::shared_ptr or any GObjects, the last person that stops needing the thing and drops its last reference, causes the GeanyDocument destructor to run which would free the memory, and you'd be 100% sure everyone that anyone referencing the document was no longer doing so by the time the actual memory gets freed. This way it ensures any weird dangling pointers or pointing to the wrong document can't happen (unless someone is holding a document pointer, without taking a reference, and without watching for its close or destroy signals, which would mean it's buggy code anyway).
It takes a different way of thinking about it, but basically "close" of a document and "free" of a document are two distinct things, although an opportune time for various document holders to drop their reference is when the document emits its "close" signal, but I could imagine some cases (like caching closed documents in a free or MRU list or something), where the reference count would never drop to zero, even though the document emitted its close signal.
This makes it a lot easier in the future to have the same document open inside multiple notebooks or windows, or even some crazy plugin thing where it's showing the document in some other type of view where it doesn't make sense for its document pointer to become invalidated even if Geany core itself has dropped its references and doesn't want to manage/show this document anymore.
This is basically the same as the ScintillaObject; parts of core or plugins are all free to retain references to it to prevent it from vanishing when the main tab showing it gets closed. I feel quite confident that this is the most sane way to manage memory/sharing of objects in C and it's endemic in G*-style code already (including all over Geany's code for other objects/widgets).
My two cents
Cheers, Matthew Brush