[MERGE] BEncode Pyrex, pt 5

[Jelmer Vernooij]

On Thu, May 28, 2009 at 07:39:04AM +0300, Alexander Belchenko wrote:
> 1) You don't keep reference to original python string passed to Decoder, but refer to internal char
> buffer. Is it safe? Is it possible situation when such string will be garbage collected while decode
> loop still working?
Yeah, that's a good point. I had removed the original reference since
it was no longer actually being used by the code itself. Fixed now.

> 2) self.tail points to internal char buffer of python object

>     cdef object _decode_int(self):
>         cdef int i
>         i = self._read_digits(c'e')
>         self.tail[i] = 0;
>         ret = PyInt_FromString(self.tail, NULL, 10)
>         self.tail[i] = c'e';

> According to python documentation you should not modify internal buffer of python string. So you're
> change it before calling PyInt_FromString ad revert it back after. What if this function raise
> exception? Would it better to use try-finally here?
Yeah, that's probably better indeed. 

> BTW, you don't need to use trailing semicolons in the pyrex sources ;-)
Bad habit :-) I've fixed them.

> 3) I'd prefer to use enum or macro constant instead of literal 32 here:

>     cdef int _encode_string(self, x) except 0:
>         cdef int n
>         self._ensure_buffer(PyString_GET_SIZE(x) + 32)
>         n = snprintf(self.tail, 32, '%d:', PyString_GET_SIZE(x))
I've added an enum.

> 4) I can't find any documentation on Py_EnterRecursiveCall/Py_LeaveRecursiveCall. Where should I look?
They're macros defined in /usr/include/pythonX.Y/ceval.h

I've attached an updated patch. Thanks for the feedback, and for doing
the original patch!

Cheers,

Jelmer
-------------- next part --------------
A non-text attachment was scrubbed...
Name: bencode-pyrex.diff
Type: text/x-diff
Size: 66721 bytes
Desc: not available
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20090528/7db1a170/attachment-0001.bin