Bazaar

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

  1. statictuple-pickling
  2. Merge into bzr.dev
Proposed by Matt Nordhoff
Status: Merged
Approved by: John A Meinel
Approved revision: no longer in the source branch.
Merged at revision: not available
Proposed branch: lp://staging/~mnordhoff/bzr/statictuple-pickling
Merge into: lp://staging/bzr
Diff against target: 112 lines
3 files modified
bzrlib/_static_tuple_c.c (+27/-3)
bzrlib/_static_tuple_py.py (+3/-0)
bzrlib/tests/test__static_tuple.py (+19/-0)
To merge this branch: bzr merge lp://staging/~mnordhoff/bzr/statictuple-pickling
Reviewer Review Type Date Requested Status
John A Meinel Approve
Review via email: mp+13720@code.staging.launchpad.net
To post a comment you must log in.
Revision history for this message
Matt Nordhoff (mnordhoff) wrote :

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

Revision history for this message
John A Meinel (jameinel) wrote :

-----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
Revision history for this message
Matt Nordhoff (mnordhoff) wrote :

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

Updating diff...

An updated diff will be available in a few minutes. Reload to see the changes.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'bzrlib/_static_tuple_c.c'
2--- bzrlib/_static_tuple_c.c 2009-10-21 14:27:00 +0000
3+++ bzrlib/_static_tuple_c.c 2009-10-21 22:09:14 +0000
4@@ -304,8 +304,8 @@
5 if (tuple_repr == NULL) {
6 return NULL;
7 }
8- result = PyString_FromFormat("%s%s", Py_TYPE(self)->tp_name,
9- PyString_AsString(tuple_repr));
10+ result = PyString_FromFormat("StaticTuple%s",
11+ PyString_AsString(tuple_repr));
12 return result;
13 }
14
15@@ -574,6 +574,29 @@
16
17
18 static PyObject *
19+StaticTuple_reduce(StaticTuple *self)
20+{
21+ PyObject *result = NULL, *as_tuple = NULL;
22+
23+ result = PyTuple_New(2);
24+ if (!result) {
25+ return NULL;
26+ }
27+ as_tuple = StaticTuple_as_tuple(self);
28+ if (as_tuple == NULL) {
29+ Py_DECREF(result);
30+ return NULL;
31+ }
32+ Py_INCREF(&StaticTuple_Type);
33+ PyTuple_SET_ITEM(result, 0, (PyObject *)&StaticTuple_Type);
34+ PyTuple_SET_ITEM(result, 1, as_tuple);
35+ return result;
36+}
37+
38+static char StaticTuple_reduce_doc[] = "__reduce__() => tuple\n";
39+
40+
41+static PyObject *
42 StaticTuple_add(PyObject *v, PyObject *w)
43 {
44 Py_ssize_t i, len_v, len_w;
45@@ -688,6 +711,7 @@
46 METH_STATIC | METH_VARARGS,
47 "Create a StaticTuple from a given sequence. This functions"
48 " the same as the tuple() constructor."},
49+ {"__reduce__", (PyCFunction)StaticTuple_reduce, METH_NOARGS, StaticTuple_reduce_doc},
50 {NULL, NULL} /* sentinel */
51 };
52
53@@ -735,7 +759,7 @@
54 PyTypeObject StaticTuple_Type = {
55 PyObject_HEAD_INIT(NULL)
56 0, /* ob_size */
57- "StaticTuple", /* tp_name */
58+ "bzrlib._static_tuple_c.StaticTuple", /* tp_name */
59 sizeof(StaticTuple), /* tp_basicsize */
60 sizeof(PyObject *), /* tp_itemsize */
61 (destructor)StaticTuple_dealloc, /* tp_dealloc */
62
63=== modified file 'bzrlib/_static_tuple_py.py'
64--- bzrlib/_static_tuple_py.py 2009-10-21 05:02:35 +0000
65+++ bzrlib/_static_tuple_py.py 2009-10-21 22:09:14 +0000
66@@ -48,6 +48,9 @@
67 def __repr__(self):
68 return '%s%s' % (self.__class__.__name__, tuple.__repr__(self))
69
70+ def __reduce__(self):
71+ return (StaticTuple, tuple(self))
72+
73 def __add__(self, other):
74 """Concatenate self with other"""
75 return StaticTuple.from_sequence(tuple.__add__(self,other))
76
77=== modified file 'bzrlib/tests/test__static_tuple.py'
78--- bzrlib/tests/test__static_tuple.py 2009-10-21 14:27:00 +0000
79+++ bzrlib/tests/test__static_tuple.py 2009-10-21 22:09:14 +0000
80@@ -16,6 +16,7 @@
81
82 """Tests for the StaticTuple type."""
83
84+import cPickle
85 import gc
86 import sys
87
88@@ -577,6 +578,24 @@
89 self.assertRaises(TypeError,
90 self.module.StaticTuple.from_sequence, foo='a')
91
92+ def test_pickle(self):
93+ st = self.module.StaticTuple('foo', 'bar')
94+ pickled = cPickle.dumps(st)
95+ unpickled = cPickle.loads(pickled)
96+ self.assertEqual(unpickled, st)
97+
98+ def test_pickle_empty(self):
99+ st = self.module.StaticTuple()
100+ pickled = cPickle.dumps(st)
101+ unpickled = cPickle.loads(pickled)
102+ self.assertIs(st, unpickled)
103+
104+ def test_pickle_nested(self):
105+ st = self.module.StaticTuple('foo', self.module.StaticTuple('bar'))
106+ pickled = cPickle.dumps(st)
107+ unpickled = cPickle.loads(pickled)
108+ self.assertEqual(unpickled, st)
109+
110 def test_static_tuple_thunk(self):
111 # Make sure the right implementation is available from
112 # bzrlib.static_tuple.StaticTuple.