Code review comment for lp:~mnordhoff/bzr/statictuple-pickling

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Matt Nordhoff wrote:
> Matt Nordhoff has proposed merging lp:~mnordhoff/bzr/statictuple-pickling into lp:bzr.
>
> Requested reviews:
> bzr-core (bzr-core)
>
>
> This adds pickling support to StaticTuple.
>
> Before this, I had written approximately 4 lines of C in my life, and had never used Python's C API. Please review it carefully. I think I actually have the error handling and reference counting right, but I really can't be sure.
>
> The Launchpad mirror for this branch is not up-to-date. Please get an up-to-date copy from my website, or look at my Loggerhead or a diff I made:
>
> http://bzr.mattnordhoff.com/loggerhead/bzr/statictuple-pickling/changes
> http://bzr.mattnordhoff.com/bzr/bzr/statictuple-pickling/statictuple-pickling-r4778.patch
>
>

 static PyObject *
+StaticTuple_reduce(StaticTuple *self)
+{
+ PyObject *result = NULL, *as_tuple = NULL;
+
+ result = PyTuple_New(2);
+ if (!result) {
+ return NULL;
+ }
+ as_tuple = StaticTuple_as_tuple(self);
+ if (as_tuple == NULL) {
+ Py_DECREF(result);
+ return NULL;
+ }
+ PyTuple_SET_ITEM(result, 0, (PyObject *)&StaticTuple_Type);
+ PyTuple_SET_ITEM(result, 1, as_tuple);
+ return result;
+}

^- PyTuple_SET_ITEM doesn't INCREF, it 'steals' a reference. This is ok
for 'as_tuple' because you are transferring ownership from
StaticTuple_as_tuple. However it is not ok for StaticTuple_Type. So you
just need to add

Py_INCREF(&StaticTuple_Type)

I think it would probably also be good to test that pickling a nested
StaticTuple works, but otherwise this looks good to me. (as long as we
add the one Py_INCREF call.)

 review: approve
 merge: approve
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkrfSSkACgkQJdeBCYSNAAN71gCg0Rnh2IJRMr5cjnTO0shD0wpz
CHQAoIA72B56GWBvg+9X9U0OLR08saz7
=MCX3
-----END PGP SIGNATURE-----