[Cython] Some small phase refactorings

[Robert Bradshaw]

On May 3, 2008, at 1:27 AM, Dag Sverre Seljebotn wrote:

>>
>> +        import Cython.Compiler.Transforms.Analysis as Analysis
>> +        Analysis.CreateFunctionScope()(self, env=env)
>> +        Analysis.AnalyseControlFlow()(self, env=env)
>> +        Analysis.AnalyseFunctionBodyDeclarations()(self, env=env)
>> +        Analysis.AnalyseFunctionBodyExpressions()(self, env=env)
>> +        options.transforms.run('after_function_analysis', self,
>> global_env=env)
>> +
>>
>> ### These look like functions, they should follow PEP 8 naming. Then
>> again,
>> ### why aren't they functions?
>
> Note that this code will almost certainly be moved again and  
> rewritten at
> some later point (they can't really belong to ModuleNode); but more
> refactoring must happen before they can be moved to their proper  
> location
> and they serve a good purpose where they are for now though.
>
> They have to be classes as they are transform objects with member  
> methods,
> some state etc. etc. But thinking about it one could have functions  
> like
> this in Analysis.py:
>
> def analyse_function_body_declarations(tree, **opts):
>     return AnalyseFunctionBodyDeclarations()(tree, **opts)
>
> if that helps. The reason I started using the __call__ is that I  
> think in
> time one can treat these as functions, like this:
>
> pipeline = [
>    f,
>    g,
>    AnalyseFunctionBodyDeclarations(),
>    Coercions(),
> ]
> for transform in pipeline:
>    tree = transform(tree)
>
> (ie, basically saying that "pipeline = f <ring> g <ring> h"...)
>
>
>> ### I'm not convinced of this one. I understand why you do it, but I
>> believe
>> ### that using the class itself rather than dispatching on a  
>> string would
>> help
>> ### here (I saw your decorator proposal - still thinking about it,  
>> but
>> might
>> ### be worth doing).
>
> If not, then the classical visitor pattern might put you at ease?:
>
> class FuncDefNode:
>     def accept(self, visitor):
>         visitor.visit_FuncDefNode(self)
>
> However, that's a lot of extra trivial code to add (this would have  
> to be
> added to _all_ classes), and when one is using Python anyway I'd  
> like to
> avoid pretending I'm writing Java... :-)

I think one could put a generic accept function in the Node class  
that would dispatch based on class name (or some class attribute) if  
one wanted, rather than having to write it for every class.

> I hope we can start using Python 2.4, then I'll implement a
> decorator/metaclass solution instead.
>
>
> In conclusion, I'd like to mention that I really think the  
> important thing
> here is to consider the "grand, large-scale" features of the patch. I
> didn't polish the details in any way, because I think that what is
> important here is the changes they make possible in the application
> structure. How the visitors look like can be changed entirely in
> Visitor.py and Analysis.py without interfering with existing code;  
> while
> the phase refactoring is going to intrude everywhere and make  
> changes all
> over the place, so the form the phase refactoring will take is the
> important point.
>
> (OTOH; I guess it is a good time to think about the details as well so
> that when the 1000 line coercion refactoring patch should be  
> written one
> knows what to do in the details...)
>
> (I've asked myself as to whether it is all worth it BTW. It is a  
> heavy and
> non-fun task. But I'm still convinced there's absolutely no way  
> around it
> if the feature-set of Cython is to grow significantly in any way. And
> realistically, it can be done in two or three days with for  
> instance me,
> you and Robert working together... So this might be too early to talk
> about it; but I end up working on it anyway because it is  
> effectively a
> blocker for me and I cannot get anywhere with my GSoC stuff until  
> it is
> done :-) )

I think this is good to talk about now. Basically, you want to change  
Cython to use the "visitor pattern" rather than the recursive pattern  
that it currently uses. This has pros and cons, but I think this  
could be a good thing, and you are certainly convinced that its  
needed to get started on the GSoC stuff. Something doesn't sit right  
about the patch though, but I can't quite put my finger on it. I  
think that it should happen at a higher level than inside the module  
node (i.e. ModuleNode should be "visited" rather than initiate the  
visitors.) As for the "phase shift" between function bodies and the  
ambient code, the ambient analysis phase needs to happen before  
entering function bodies, but I think it could be done immediately  
after at the tail end of the same phase (so all declarations would be  
analyzed before any times need to be looked at) which may make things  
cleaner.

Probably http://wiki.cython.org/enhancements/treevisitors should be  
fleshed out with more specific details and a plan.

- Robert