Merge lp:~mnordhoff/bzr/statictuple-pickling into lp:bzr

[Matt Nordhoff]

[Matt Nordhoff (mnordhoff)]

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

[John A Meinel (jameinel)]

-----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-----

review: Approve

[Matt Nordhoff (mnordhoff)]

John A Meinel wrote:
> ^- 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.)

Done and done. Thank you! :-)

I thought the test was getting a little unwieldy, so I split it up into
3 methods. I hope you don't mind.

Incremental diff:

<http://bzr.mattnordhoff.com/loggerhead/bzr/statictuple-pickling/revision/4779>
--

=== modified file 'bzrlib/_static_tuple_c.c'
--- bzrlib/_static_tuple_c.c	2009-10-21 14:27:00 +0000
+++ bzrlib/_static_tuple_c.c	2009-10-21 22:09:14 +0000
@@ -304,8 +304,8 @@
     if (tuple_repr == NULL) {
         return NULL;
     }
-    result = PyString_FromFormat("%s%s", Py_TYPE(self)->tp_name,
-                                         PyString_AsString(tuple_repr));
+    result = PyString_FromFormat("StaticTuple%s",
+                                 PyString_AsString(tuple_repr));
     return result;
 }
 
@@ -574,6 +574,29 @@
 
 
 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;
+    }
+    Py_INCREF(&StaticTuple_Type);
+    PyTuple_SET_ITEM(result, 0, (PyObject *)&StaticTuple_Type);
+    PyTuple_SET_ITEM(result, 1, as_tuple);
+    return result;
+}
+
+static char StaticTuple_reduce_doc[] = "__reduce__() => tuple\n";
+
+
+static PyObject *
 StaticTuple_add(PyObject *v, PyObject *w)
 {
     Py_ssize_t i, len_v, len_w;
@@ -688,6 +711,7 @@
      METH_STATIC | METH_VARARGS,
      "Create a StaticTuple from a given sequence. This functions"
      " the same as the tuple() constructor."},
+    {"__reduce__", (PyCFunction)StaticTuple_reduce, METH_NOARGS, StaticTuple_reduce_doc},
     {NULL, NULL} /* sentinel */
 };
 
@@ -735,7 +759,7 @@
 PyTypeObject StaticTuple_Type = {
     PyObject_HEAD_INIT(NULL)
     0,                                           /* ob_size */
-    "StaticTuple",                               /* tp_name */
+    "bzrlib._static_tuple_c.StaticTuple",        /* tp_name */
     sizeof(StaticTuple),                         /* tp_basicsize */
     sizeof(PyObject *),                          /* tp_itemsize */
     (destructor)StaticTuple_dealloc,             /* tp_dealloc */

=== modified file 'bzrlib/_static_tuple_py.py'
--- bzrlib/_static_tuple_py.py	2009-10-21 05:02:35 +0000
+++ bzrlib/_static_tuple_py.py	2009-10-21 22:09:14 +0000
@@ -48,6 +48,9 @@
     def __repr__(self):
         return '%s%s' % (self.__class__.__name__, tuple.__repr__(self))
 
+    def __reduce__(self):
+        return (StaticTuple, tuple(self))
+
     def __add__(self, other):
         """Concatenate self with other"""
         return StaticTuple.from_sequence(tuple.__add__(self,other))

=== modified file 'bzrlib/tests/test__static_tuple.py'
--- bzrlib/tests/test__static_tuple.py	2009-10-21 14:27:00 +0000
+++ bzrlib/tests/test__static_tuple.py	2009-10-21 22:09:14 +0000
@@ -16,6 +16,7 @@
 
 """Tests for the StaticTuple type."""
 
+import cPickle
 import gc
 import sys
 
@@ -577,6 +578,24 @@
         self.assertRaises(TypeError,
                           self.module.StaticTuple.from_sequence, foo='a')
 
+    def test_pickle(self):
+        st = self.module.StaticTuple('foo', 'bar')
+        pickled = cPickle.dumps(st)
+        unpickled = cPickle.loads(pickled)
+        self.assertEqual(unpickled, st)
+
+    def test_pickle_empty(self):
+        st = self.module.StaticTuple()
+        pickled = cPickle.dumps(st)
+        unpickled = cPickle.loads(pickled)
+        self.assertIs(st, unpickled)
+
+    def test_pickle_nested(self):
+        st = self.module.StaticTuple('foo', self.module.StaticTuple('bar'))
+        pickled = cPickle.dumps(st)
+        unpickled = cPickle.loads(pickled)
+        self.assertEqual(unpickled, st)
+
     def test_static_tuple_thunk(self):
         # Make sure the right implementation is available from
         # bzrlib.static_tuple.StaticTuple.