Stefan Sperling <stsp_at_elego.de> писал в своём письме Thu, 01 Oct 2009
21:02:03 +0400:
> On Sun, Sep 27, 2009 at 01:21:10PM -0700, Roman Donchenko wrote:
>> Author: rdonch
>> Date: Sun Sep 27 13:21:10 2009
>> New Revision: 39635
>>
>> Log:
>> SWIG/Python: add a typemap for APR arrays of svn_opt_revision_range_t *.
>> This fixes svn.client.log5 and probably svn.client.merge_peg3 too.
>
> I've reviewed this since it is proposed for backport.
>
> Some comments:
>
>> ---
>> trunk/subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c Sat
>> Sep 26 17:19:37 2009 (r39634)
>> +++
>> trunk/subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c Sun
>> Sep 27 13:21:10 2009 (r39635)
>> @@ -1248,6 +1248,51 @@ const apr_array_header_t *svn_swig_py_re
>> return temp;
>> }
>>
>> +const apr_array_header_t *
>> +svn_swig_py_struct_ptr_list_to_array(PyObject *source,
>> + swig_type_info * type_descriptor,
>> + apr_pool_t *pool)
>> +{
>> + int targlen;
>> + apr_array_header_t *temp;
>> +
>> + if (source == Py_None)
>> + return NULL;
>> +
>> + if (!PySequence_Check(source))
>> + {
>> + PyErr_SetString(PyExc_TypeError, "not a sequence");
>> + return NULL;
>> + }
>> + targlen = PySequence_Length(source);
>
> Can targlen become negative here in case of errors?
> Does not look like it but I don't know the python C API.
Well, I don't think it can happen for builtin Python sequences, but I
suppose it's a possibility if we are passed a custom sequence type which
throws an exception in its __len__ method.
Actually, while seeking this answer, I discovered another corner case —
PySequence_Length returns a Py_ssize_t, which can be bigger than an int.
So if the user passes more than INT_MAX elements, bad things will happen.
I will add the check for both conditions, albeit I'm not sure if it should
be backported, since the conditions are fairly implausible and I will have
to modify more than just this function.
>
>> + temp = apr_array_make(pool, targlen, sizeof(void *));
>> +
>> + temp->nelts = targlen;
>
> Why initialise temp->nelts explicitly?
> AFAIK the APR API takes care of this internally.
Not really, APR arrays start with zero elements.
>
>> + while (targlen--)
>
> You could use a "for (i = 0; i < targlen; i++)" loop here, and ...
>
>> + {
>> + void * struct_ptr;
>> + int status;
>> + PyObject *o = PySequence_GetItem(source, targlen);
>
> use 'i' as an index here and avoid using 'targlen' as an index (which
> should really be a separate variable such as 'i' but maybe that's just
> me...)
I must admit I don't like the backwards loop much myself. I adapted (read:
copy-pasted) this function from a similar one, and didn't feel like
changing the loop structure "just because".
>
>> + if (o == NULL)
>> + return NULL;
>> +
>> + status = svn_swig_ConvertPtr(o, &struct_ptr, type_descriptor);
>> +
>> + if (status == 0)
>> + {
>> + APR_ARRAY_IDX(temp, targlen, void *) = struct_ptr;
>
> You could use APR_ARRAY_PUSH(temp, void *) = struct_ptr; here.
> That would align better with common use of arrays in our code,
> PUSH is usually used for writing to the array, IDX for retrieval.
Answered in the first sub-thread.
Roman.
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2403043
Received on 2009-10-02 21:07:23 CEST