On 6/18/06, Jelmer Vernooij <jelmer@samba.org> wrote:
> On Sun, 2006-06-18 at 01:25 -0700, David James wrote:
> > On 6/17/06, Jelmer Vernooij <jelmer@samba.org> wrote:
> > 2) In svn_swig_py_ra_file_rev_handler_func, we should pass in
> > py_prop_diffs to the callback function instead of the regular
> > "prop_diffs". (prop_diffs is not a Python object.)
> Well spotted, thanks.
>
> > Once these issues are fixed, this is ready to commit, I think.
> Fixed version is attached. Is this ok to commit?
Looks great! Thanks for adding the extra NULL checks to
svn_swig_py_ra_file_rev_handler_func. These checks make our code much
more robust.
A few more review comments:
1. If you add Py_None to an array, you should Py_INCREF it first.
Otherwise, the Py_None variable might be destroyed by the Python
garbage collector when your array is destroyed, causing some mayhem.
2. PyDict_New() can return NULL. We should check for this explicitly.
3. The new svn_swig_py_proparray_to_dict function is not declared in
swigutil_py.h (or used elsewhere in the Python bindings), so it should
be marked as 'static', and given an appropriate name. I renamed the
function to 'proparray_to_dict'.
I also have a few style suggestions:
4. In svn_swig_py_proparray_to_dict, py_key won't be NULL, because you
check for it explicitly. So you can use Py_DECREF here instead of
Py_XDECREF for clarity.
5. There's a few places where the braces are off in your patch, there
are trailing spaces, or the lines are longer than 80 chars.
6. It's nice to put some whitespace between the different files in
your log message.
Since the above comments are only minor, I fixed all of them and
committed in r20158.
Cheers,
David
P.S. NOTE: There are many places in the Python bindings where we don't
check whether the return value of PyList_New are NULL. These are all
segfault bugs which will be sensitized if you run out of memory.
--
David James -- http://www.cs.toronto.edu/~james
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sun Jun 18 18:34:38 2006