[Cython] Bug in Cython producing incorrect C code

[Vitja Makarov]

2012/2/12 mark florisson <markflorisson88 at gmail.com>:
> On 12 February 2012 14:06, Vitja Makarov <vitja.makarov at gmail.com> wrote:
>> 2012/2/4 Vitja Makarov <vitja.makarov at gmail.com>:
>>> 2012/1/26 mark florisson <markflorisson88 at gmail.com>:
>>>> On 26 January 2012 19:27, Stefan Behnel <stefan_ml at behnel.de> wrote:
>>>>> mark florisson, 26.01.2012 20:15:
>>>>>> On 26 January 2012 18:53, Stefan Behnel wrote:
>>>>>>> mark florisson, 26.01.2012 16:20:
>>>>>>>> I think this problem can trivially be solved by creating a ProxyNode
>>>>>>>> that should never be replaced by any transform, but it's argument may
>>>>>>>> be replaced. So you wrap self.rhs in a ProxyNode and use that to
>>>>>>>> create your CloneNodes.
>>>>>>>
>>>>>>> I can't see what a ProxyNode would do that a CloneNode shouldn't do anyway.
>>>>>>
>>>>>> It wouldn't be a replacement, merely an addition (an extra indirection).
>>>>>
>>>>> What I was trying to say was that a ProxyNode would always be required by a
>>>>> CloneNode, but I don't see where a ProxyNode would be needed outside of a
>>>>> CloneNode. So it seems rather redundant and I don't know if we need a
>>>>> separate node for it.
>>>>
>>>> Yes it would be needed only for that, but I think the only real
>>>> alternative is to not use CloneNode at all, i.e. make the
>>>> transformation Dag mentioned, where you create new rhs (NameNode?)
>>>> references to the temporary result.
>>>>
>>>
>>> Now it seems to be the only case when we got problem like this. It
>>> means that clones may be safely created at very late stage.
>>> So transforming CascadeAssignment into SingleAssignments doesn't solve
>>> generic problem.
>>>
>>> I tried to implement conditional inlining the same problem may happen
>>> there (ConditionalCallNode owns arguments and replaces
>>> SimpleCallNode's args with clones). Splitting analyse_expressions()
>>> would help. On the other hand moving this optimization after
>>> OptimizeBuiltinCalls() would help too.
>>>
>>
>> I tried to introduce finalize_expressions() here:
>>
>> https://github.com/vitek/cython/tree/_finalize_expressions
>>
>> I moved arg_tuple creation logic from SimpleCallNode's analyse_types()
>> to finalize_expressions() so few tests are broken now.
>>
>> Now inlining is done right before AnalyseExpressions before arg_tuple
>> is created (before pyobject coercion nodes are created). It must be
>> run after expression analysis. So I'm completely sure that
>> analyse_types() must be split.
>
> Ah, I didn't realize you were working on that, I fixed the cascaded
> assignment bug and pushed it to master a while ago. Anyway, if this
> fixes the problem for cascaded assignment, feel free to do a hard
> reset to remove those commits (probably want to keep the added tests
> though).
>

Nice, I'm ok with your fix! Btw finalize_expressions() isn't done yet.
It's a major change so it's better to wait until next release.

-- 
vitja.