Author novalis_dt
Recipients nnorwitz, novalis_dt
Date 2008-11-06.22:29:33
SpamBayes Score 2.77556e-16
Marked as misclassified No
Message-id <>
Neal Norwitz <> added the comment:
> Interesting approach.  I was surprised to see the change to the AST, 
> but I understand why you did it.  I think if the AST optimization 
> approach works out well, we will want to have some more general 
> mechanism to communicate these optimization (or other!) hints to the 
> compiler.  It would suck to have to modify the AST each time we 
> wanted to add a new optimization.  You and Tom might have better 
> ideas for how best to achieve that.

I really didn't want to have to change the AST.  The problem was that
there was a feature of the Python bytecode which was not representable
in Python source code.  I don't anticipate future optimizations
requiring changes to the AST, because I now believe every important
feature of the bytecode is representable in the AST.  The one exception
might be if we have a need for arbitrary jumps.  I don't think that
would be useful, but it might conceivably.  In that case, we would need
Goto and Label AST nodes.  

The thing that struck me as ugly was the idea that there's one object
(the optimizer) that knows everything about all optimization operations.
 This seems like a great case for the visitor pattern.  The optimizer
ought to have a list of functions to call for each type of AST node,
each with some private storage space.  But I didn't want to make that
kind of dramatic change without consulting with Tom.

> I'll make some comments that would need to be addressed, but you might
> want to wait making any changes depending on what approach you decide 
> to take.
> The most important missing piece is tests to show that the new code 
> works.

> There are various whitespace issues.  Both whitespace only changes 
> that should be avoided and indentation inconsistencies with the 
> existing code (or so it appears).  The latter could be the way I'm 
> viewing the file or existing problems with tabs.

Fixed, I think.  The original code appears to be somewhat inconsistent
between files in its uses of spaces/tabs, but I think I have now matched
the style of each file.

> In Python/optimize.c, you need to handle the case where the 
> PyDict_New() calls fail.  It looks like currently an undetected error
> can happen during construction.  And on destruction it will crash 
> because the objects will be NULL when calling Py_DECREF.

> All calls like PyDict_SetItem(), PyInt_FromLong(), etc need to handle
> errors.

I'm not exactly sure which "etc" need to handle errors.  Py*_As*? 
Anyway, I changed the ones you mentioned.

> I'll need to study the code a lot more to see how well it behaves. 
> Tests would help a lot with that.

I've added a few.
Date User Action Args
2008-11-06 22:30:42novalis_dtsetrecipients: + novalis_dt, nnorwitz
2008-11-06 22:30:42novalis_dtsetmessageid: <>
2008-11-06 22:29:42novalis_dtlinkissue4264 messages
2008-11-06 22:29:39novalis_dtcreate