Bazaar

Code review comment for lp:~jameinel/bzr/2.1-static-tuple-btree-string-intern

  1. 2.1-static-tuple-btree-string-intern
  2. Merge into bzr.dev
Revision history for this message
Andrew Bennetts (spiv) wrote :

[...]
> > In plain Python you could define an __radd__ for this, so surely there's a
> > way to do this in C?
 [...]
> > You may need to do something odd like provide the nb_add slot, even though
> > this isn't really a numeric type, but I think that's ok. (All pure python
> > classes would have that I think, even the non-numeric ones, so presumably
> > having tp_as_number filled doesn't automatically make Python do dumb
> > things.)

By the way, because lack of support for tuple + StaticTuple caused all the
babune builders to go red, I took a look at this. (specifically,
bzrlib.tests.per_repository.test_write_group.TestResumeableWriteGroup.test_commit_resumed_write_group_with_missing_parents
was failing)

You actually need nb_coerce as well as nb_add. Here's a rough patch that does
this. The error it gives when you try to add a plain tuple with incompatible
elements (e.g. ints) is probably not ideal, but it works.

-Andrew.

static_tuple_add.patch
1=== modified file 'bzrlib/_static_tuple_c.c'
2--- bzrlib/_static_tuple_c.c 2009-10-15 18:18:44 +0000
3+++ bzrlib/_static_tuple_c.c 2009-10-16 08:24:36 +0000
4@@ -513,6 +513,78 @@
5 "Check to see if this tuple has been interned.\n";
6
7
8+int
9+StaticTuple_coerce(PyObject **v, PyObject **w)
10+{
11+ StaticTuple *st;
12+ if (PyTuple_Check(*v)) {
13+ st = (StaticTuple*) StaticTuple_new_constructor(
14+ &StaticTuple_Type, *v, NULL);
15+ if (!st)
16+ return -1;
17+ Py_INCREF(st);
18+ *v = (PyObject*)st;
19+ } else if (StaticTuple_CheckExact(*v))
20+ Py_INCREF(*v);
21+ else
22+ return 1;
23+
24+ if (PyTuple_Check(*w)) {
25+ st = (StaticTuple*) StaticTuple_new_constructor(
26+ &StaticTuple_Type, *w, NULL);
27+ if (!st)
28+ return -1;
29+ Py_INCREF(st);
30+ *w = (PyObject*)st;
31+ } else if (StaticTuple_CheckExact(*w))
32+ Py_INCREF(*w);
33+ else
34+ return 1;
35+ return 0;
36+}
37+
38+static PyObject *
39+StaticTuple_add(PyObject *v, PyObject *w)
40+{
41+ PyObject *v_t = NULL, *w_t = NULL;
42+ PyObject *tmp_tuple, *result;
43+ /* StaticTuples and plain tuples may be added (concatenated) to
44+ * StaticTuples.
45+ */
46+ if (StaticTuple_CheckExact(v)) {
47+ v_t = StaticTuple_as_tuple((StaticTuple*)v);
48+ if (!v_t)
49+ goto fail;
50+ } else if (PyTuple_Check(v))
51+ v_t = v;
52+ else
53+ goto not_imp;
54+
55+ if (StaticTuple_CheckExact(w)) {
56+ w_t = StaticTuple_as_tuple((StaticTuple*)w);
57+ if (!w_t)
58+ goto fail;
59+ } else if (PyTuple_Check(w))
60+ w_t = w;
61+ else
62+ goto not_imp;
63+
64+ tmp_tuple = PySequence_Concat(v_t, w_t);
65+ result = StaticTuple_new_constructor(&StaticTuple_Type, tmp_tuple, NULL);
66+ Py_DECREF(tmp_tuple);
67+ Py_INCREF(result);
68+ return result;
69+
70+not_imp:
71+ Py_XDECREF(v_t);
72+ Py_XDECREF(w_t);
73+ return Py_NotImplemented;
74+fail:
75+ Py_XDECREF(v_t);
76+ Py_XDECREF(w_t);
77+ return NULL;
78+}
79+
80 static PyObject *
81 StaticTuple_item(StaticTuple *self, Py_ssize_t offset)
82 {
83@@ -574,6 +646,29 @@
84 {NULL, NULL} /* sentinel */
85 };
86
87+
88+static PyNumberMethods StaticTuple_as_number = {
89+ (binaryfunc) StaticTuple_add, /* nb_add */
90+ 0, /* nb_subtract */
91+ 0, /* nb_multiply */
92+ 0, /* nb_divide */
93+ 0, /* nb_remainder */
94+ 0, /* nb_divmod */
95+ 0, /* nb_power */
96+ 0, /* nb_negative */
97+ 0, /* nb_positive */
98+ 0, /* nb_absolute */
99+ 0, /* nb_nonzero */
100+ 0, /* nb_invert */
101+ 0, /* nb_lshift */
102+ 0, /* nb_rshift */
103+ 0, /* nb_and */
104+ 0, /* nb_xor */
105+ 0, /* nb_or */
106+ StaticTuple_coerce, /* nb_coerce */
107+};
108+
109+
110 static PySequenceMethods StaticTuple_as_sequence = {
111 (lenfunc)StaticTuple_length, /* sq_length */
112 0, /* sq_concat */
113@@ -604,7 +699,7 @@
114 0, /* tp_setattr */
115 0, /* tp_compare */
116 (reprfunc)StaticTuple_repr, /* tp_repr */
117- 0, /* tp_as_number */
118+ &StaticTuple_as_number, /* tp_as_number */
119 &StaticTuple_as_sequence, /* tp_as_sequence */
120 0, /* tp_as_mapping */
121 (hashfunc)StaticTuple_hash, /* tp_hash */

« Back to merge proposal