Port all reference counting to the C++ part
ncordon opened this issue · 2 comments
Right now NodeIterator (node_iterator.py file) and Node (node.py file) contain a reference to the Context, so that it does not get deallocated when we are working with an iterator which traverses the tree, for example.
For the NodeIterator to be able to call iterate method we have to create an iterator in the C++ side, which right now does not do a Py_INCREF of any context, because the method call does not include any context (and in fact, for internal nodes we do not want for it to receive any, since it creates a free context and attaches it to the iterator). We may want to separate both of the methods (create an iterator from the C++ side for internal and external nodes), and add the Py_INCREF for contexts and external references (and the corresponding Py_DECREF in the
PyUastIter_dealloc method).
The same changes (about the Py_INCREF and Py_DECREF for the context) should be introduced in PythonContextExt_filter and PythonContext_filter. Right now we cannot introduce the former ones because the _filter methods also create a PyUastIter object (as PyUastIter_new does). And we have explained that PyUastIter_new is not receiving a context, so we cannot Py_DECREF the context in the deallocation of PyUastIter, and therefore we cannot Py_INCREF the context in _filter methods, if we do not want to leak memory).
It may seem that this is a problem related to design of Node and NodeIterator. If we want to get rid of the references to ctx in those classes, then we should move the iterate functionality entirely to ResultContext, but we have to think this through because right now having an iterate method in Node enhances ease of use, being able to call node.iterate(TreeOrder.PRE_ORDER), for example. This can ease the introduction of the missing Py_INCREFs and Py_DRECREFs but it is not mandatory to do.
This seems like a really good idea. Also, this is a really great detailed write-up of the problem, thank you.
I belive the same context lifetime considerations for iterators are applicable for scala-client as well.