On Sat, Feb 7, 2009 at 10:08 AM, Stefan Behnel <[email protected]> wrote:
>
> Lisandro Dalcin wrote:
>>>> The only thing that I'm not completely sure about is the
>>>> implementation of this, though it works fine in my 32bit Linux box
>>>>
>>>> static INLINE size_t __pyx_PyInt_AsSize_t(PyObject* b);
>>
>> http://trac.cython.org/cython_trac/attachment/ticket/207/SIZE_T.diff#preview
>
> static INLINE size_t __pyx_PyInt_AsSize_t(PyObject* b) {
> unsigned PY_LONG_LONG val =
> __pyx_PyInt_AsUnsignedLongLong(b);
> if (unlikely(val == (unsigned PY_LONG_LONG)-1 &&
> PyErr_Occurred())) {
> return (size_t)-1;
>
> The "(size_t)-1" definitely looks funny. I noticed that "(unsigned
> PY_LONG_LONG)-1" is also used in __pyx_PyInt_AsUnsignedLongLong(), so I
> just hope it does the right thing.
>
That's the way things are implemented in core CPython, and AFAIK it
should works, at least if Cython-generated C code is compiled with the
same C compiler as core CPython library. I do not know I the C
standard is explicit about what you get when you cast a negative
integer to an unsigned integer.
>
> } else if (unlikely((unsigned PY_LONG_LONG)(size_t)val
> != val)) {
> PyErr_SetString(PyExc_OverflowError, "value too
> large to convert to size_t");
>
> This should return an error value, so it lacks a "return (size_t)-1".
>
OK, I'll fix that ...
>
> It would be great if we had tests for the error cases. It's not obvious
> that they work as expected. The same applies to the "unsigned long long"
> conversion, IMHO.
>
I've added a testcase for overflowing 'size_t' conversions. But yes,
such testcases should be implemented for all integral types... I'll
give a try next week in some spare time.
>
>>>> Of course, this change is not backward compatible... Cython will not
>>>> compile the code:
>>>>
>>>> cdef extern from "stdlib.h":
>>>> ctypedef unsigned long size_t
>>> Is this because of an explicit error, or just a side effect of the
>>> implementation?
>>>
>>
>> Cython generates the following error (IMHO, that's a good thing)
>>
>> Error converting Pyrex file to C:
>> ------------------------------------------------------------
>> ...
>> #cython: embedsignature=True
>> ctypedef unsigned long size_t
>> ^
>> ------------------------------------------------------------
>>
>> /u/dalcinl/Devel/mpi4py-dev/src/mpi4py.MPI.pyx:2:29: Empty declarator
>
> Not really the most telling error message, but not cryptic either. Fine
> with me.
>
>
>>>> What should be the behavior if the user tries to ctypedef 'Py_ssize_t'
>>>> or 'size_t', provided that Cython treats them as pre-defined C types?
>>> I didn't care at the time when I implemented the Py_ssize_t stuff, but when
>>> you ask now, I prefer getting at least a warning. I doubt that Cython
>>> handles a "ctypedef ... int" gracefully, for example.
>>
>> Sorry for the noise, I got confused.. Cython DO FAIL if you 'ctypedef
>> ... Py_ssize_t'.
>
> Same as above, I assume: it thinks that you are still collecting types and
> modifiers, so it's missing the new type name at the end.
>
>
>>>> Finally, can I push the changes?
>>> Could you file a bug report for now (if only for documentation purposes)
>>> and post the patch there? We should be a bit careful with major changes by
>>> now, as we are currently trying to stabilise Cython for the 0.11 release.
>>>
>>
>> OK. Just hope this can get in for the next release.
>
> Apart from what I said above, it looks ok, so I'd like to see it in as well.
>
> Stefan
>
> _______________________________________________
> Cython-dev mailing list
> [email protected]
> http://codespeak.net/mailman/listinfo/cython-dev
>
--
Lisandro Dalcín
---------------
Centro Internacional de Métodos Computacionales en Ingeniería (CIMEC)
Instituto de Desarrollo Tecnológico para la Industria Química (INTEC)
Consejo Nacional de Investigaciones Científicas y Técnicas (CONICET)
PTLC - Güemes 3450, (3000) Santa Fe, Argentina
Tel/Fax: +54-(0)342-451.1594
_______________________________________________
Cython-dev mailing list
[email protected]
http://codespeak.net/mailman/listinfo/cython-dev
