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.
> + 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.
> + 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...)
> + 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.
> + Py_DECREF(o);
> + }
Thanks,
Stefan
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2402609
Received on 2009-10-01 19:02:22 CEST