Robert Bradshaw, 27.01.2011 07:39:
> On Wed, Jan 26, 2011 at 10:29 PM, Stefan Behnel wrote:
>> ticket #654 describes a problem with the order in which function call
>> arguments are evaluated. I fixed it and it broke Sage.
>
> Thanks for fixing this, I never got around to it last night. I'm
> wondering how many other similar situations may be lurking around.
I could imagine that there are cases where comparisons of coerced types run
into the same issue. And other kinds of expressions may also suffer from this.
> Of
> course we could store every sub-expression in temporary variables, but
> that would be less than ideal.
We will be getting close to that, though, look at this:
-----------------
cdef object X = 1
cdef redefine_global():
global X
x,X = X,2
return x
cdef call3(object x1, int o, object x2):
return (x1, o, x2)
def test_global_redefine():
"""
>>> test_global_redefine()
(1, 1, 2)
"""
return call3(X, redefine_global(), X)
-----------------
The same applies to any non-local or closure variable. NameNode currently
just says this:
def is_simple(self):
# If it's not a C variable, it'll be in a temp.
return 1
True when you interpret "simple" as "side-effect free", but false if you
take it to mean "has no external dependencies". Maybe we need something
like "coerce_to_local()" or "coerce_to_owned()"?
>> The problem was that in cases where some arguments are simple and others
>> are not, the non-simple arguments are stuffed into a temp before hand, thus
>> being evaluated before the other arguments. This can break side-effects of
>> simple arguments, which include C function calls.
>>
>> My fix was to put all arguments into temps if any of them are in temps
>> anyway. However, there is this code in Sage (wrapper_rr.pyx/.pxd):
>>
>> cdef class Wrapper_rr(Wrapper):
>> cdef int _n_args
>> cdef mpfr_t* _args
>> ...
>>
>> # offending line:
>> mpfr_init2(self._args[i], self.domain.prec())
>>
>> The signature of mpfr_init2() is
>>
>> ctypedef __mpfr_struct* mpfr_t
>> void mpfr_init2 (mpfr_t x, mp_prec_t prec)
>>
>> Cython generates this code:
>>
>> mpfr_t __pyx_t_7;
>> ...
>> __pyx_t_7 = (((struct ...)__pyx_v_self)->_args[__pyx_v_i]);
>> ...
>> mpfr_init2(__pyx_t_7, __pyx_t_8);
>>
>> Looks reasonable at first sight. However, gcc complains about the temp
>> assignment:
>>
>> sage/ext/interpreters/wrapper_rr.c:2737: error: incompatible types in
>> assignment
>>
>> Any idea what might be going wrong here?
>
> mpfr_t is actually an __mpfr_struct[1]. (It's a clever hack to have
> stack-allocated, pass-by-reference semantics.) We only need to pull
> non-simple arguments into temp variables.
Ah, that would explain it. But this also means that there may be cases
where we can't help breaking existing code with such a change.
Stefan
_______________________________________________
Cython-dev mailing list
[email protected]
http://codespeak.net/mailman/listinfo/cython-dev
