You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Stefan Sperling <st...@elego.de> on 2009/10/01 17:02:03 UTC
Re: svn commit: r39635 - in trunk/subversion/bindings/swig: .
include python/libsvn_swig_py
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
Re: svn commit: r39635 - in trunk/subversion/bindings/swig: .
include python/libsvn_swig_py
Posted by Roman Donchenko <DX...@yandex.ru>.
Stefan Sperling <st...@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
Re: svn commit: r39635 - in trunk/subversion/bindings/swig: .
include python/libsvn_swig_py
Posted by Julian Foad <ju...@btopenworld.com>.
On Thu, 2009-10-01 at 18:17 +0100, Stefan Sperling wrote:
> On Thu, Oct 01, 2009 at 06:11:48PM +0100, Julian Foad wrote:
> > Stefan Sperling wrote:
> > > > + 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.
> >
> > No, PUSH is different: it means extend the array by one element and then
> > set that element.
>
> Oh! Right, then I was mistaken. So in this case, PUSH would only be
> appropriate if the array was created with 0 elements initially.
Ahh... I guess that explains the initial "temp->nelts = targlen;" that
you queried. APR doc strings aren't clear on whether "apr_array_make"
sets nelts to the given value or just determines the amount of memory
allocated.
- Julian
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2402622
Re: svn commit: r39635 - in trunk/subversion/bindings/swig: .
include python/libsvn_swig_py
Posted by Stefan Sperling <st...@elego.de>.
On Thu, Oct 01, 2009 at 06:17:02PM +0100, Stefan Sperling wrote:
> On Thu, Oct 01, 2009 at 06:11:48PM +0100, Julian Foad wrote:
> > Stefan Sperling wrote:
> > > > + 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.
> >
> > No, PUSH is different: it means extend the array by one element and then
> > set that element.
>
> Oh! Right, then I was mistaken. So in this case, PUSH would only be
> appropriate if the array was created with 0 elements initially.
Wait a minute... that cannot be right.
We're creating arrays with more than 0 elements all over the place
and use PUSH to populate them.
Turns out that new elements are only added as-needed:
/**
* Add a new element to an array (as a first-in, last-out stack)
* @param arr The array to add an element to.
* @return Location for the new element in the array.
* @remark If there are no free spots in the array, then this function will
* allocate new space for the new element.
*/
APR_DECLARE(void *) apr_array_push(apr_array_header_t *arr);
[...]
#define APR_ARRAY_PUSH(ary,type) (*((type *)apr_array_push(ary)))
So I guess using PUSH in that loop is fine after all.
Stefan
--
printf("Eh???/n");
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2402621
Re: svn commit: r39635 - in trunk/subversion/bindings/swig: .
include python/libsvn_swig_py
Posted by Stefan Sperling <st...@elego.de>.
On Thu, Oct 01, 2009 at 06:11:48PM +0100, Julian Foad wrote:
> Stefan Sperling wrote:
> > > + 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.
>
> No, PUSH is different: it means extend the array by one element and then
> set that element.
Oh! Right, then I was mistaken. So in this case, PUSH would only be
appropriate if the array was created with 0 elements initially.
So IDX is fine, but using a separate variable for the index would
still be nice :)
Thanks,
Stefan
--
printf("Eh???/n");
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2402618
Re: svn commit: r39635 - in trunk/subversion/bindings/swig: .
include python/libsvn_swig_py
Posted by Julian Foad <ju...@btopenworld.com>.
Stefan Sperling wrote:
> > + 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.
No, PUSH is different: it means extend the array by one element and then
set that element.
- Julian
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2402614