Why did you suggest that PyAST_FromNode returns PyTypeObject*?
I can't see why type objects are much used in the AST, unless
I'm missing something essential.
Anyway, I started converting ast.c (two functions only), and
noticed that there is a convention to have nested variables
referring to fresh memory (e.g. inside switch statements). I
started changing these to have all variables at the toplevel.
Then, in either success or failure, you have to release all
of them. Unfortunately, sometimes in failure, an additional
function is called, which isn't called in success. So I
added a success: label.
Also, it is somewhat inconvenient that PyList_SET_ITEM
steals references. Currently, I INCREF the objects added
to the list (as the success: label will DECREF them);
alternatively, clearing the pointer to NULL might also
be appropriate. Perhaps we could have a STEAL_ITEM
macro inside ast.c:
#define STEAL_ITEM(list,index,variable) \
Transferring part of the discussion of Thomas Lee's PEP 341 patch to
python-dev. . .
Neal Norwitz wrote in the SF patch tracker:
> Thomas, I hope you will write up this experience in coding
> this patch. IMO it clearly demonstrates a problem with the
> new AST code that needs to be addressed. ie, Memory
> management is not possible to get right. I've got a 700+
> line patch to ast.c to correct many more memory issues
> (hopefully that won't cause conflicts with this patch). I
> would like to hear ideas of how the AST code can be improved
> to make it much easier to not leak memory and be safe at the
> same time.
As Neal pointed out, it's tricky to write code for the AST parser and compiler
without accidentally letting memory leak when the parser or compiler runs into
a problem and has to bail out on whatever it was doing. Thomas's patch got to
v5 (based on Neal's review comments) with memory leaks still in it, my review
got rid of some of them, and we think Neal's last review of v6 of the patch
got rid of the last of them.
I am particularly concerned about the returns hidden inside macros in the AST
compiler's symbol table generation and bytecode generation steps. At the
moment, every function in compile.c which allocates code blocks (or anything
else for that matter) and then calls one of the VISIT_* macros is a memory
leak waiting to happen.
Something I've seen used successfully (and used myself) to deal with similar
resource-management problems in C code is to use a switch statement, rather
than getting goto-happy.
Specifically, the body of the entire function is written inside a switch
statement, with 'break' then used as the equivalent of "raise Exception". For
/* Real function body goes here */
/* Error cleanup code goes here */
It avoids the potential for labelling problems that arises when goto's are
used for resource cleanup. It's a far cry from real exception handling, but
it's the best solution I've seen within the limits of C.
A particular benefit comes when macros which may abort function execution are
used inside the function - if those macros are rewritten to use break instead
of return, then the function gets a chance to clean up after an error.
P.S. Getting rid of the flow control macros entirely is another option, of
course, but it would make compile.c and symtable.c a LOT harder to follow.
Raymond Chen's articles notwithstanding, a preprocessor-based mini-language
does make sense in some situations, and I think this is one of them.
Particularly since the flow control macros are private to the relevant
Nick Coghlan | ncoghlan(a)gmail.com | Brisbane, Australia