Implement the intention of SF patch 472523 (but coded differently).
In the past, an object's tp_compare could return any value. In 2.2
the docs were tightened to require it to return -1, 0 or 1; and -1 for
an error.
We now issue a warning if the value is not in this range. When an
exception is raised, we allow -1 or -2 as return value, since -2 will
the recommended return value for errors in the future. (Eventually
tp_compare will also be allowed to return +2, to indicate
NotImplemented; but that can only be implemented once we know all
extensions return a value in [-2...1]. Or perhaps it will require the
type to set a flag bit.)
I haven't decided yet whether to backport this to 2.2.x. The patch
applies fine. But is it fair to start warning in 2.2.2 about code
that worked flawlessly in 2.2.1?
diff --git a/Objects/object.c b/Objects/object.c
index b1bf2c3..3207d8a 100644
--- a/Objects/object.c
+++ b/Objects/object.c
@@ -353,6 +353,46 @@
#endif
+/* Helper to warn about deprecated tp_compare return values. Return:
+ -2 for an exception;
+ -1 if v < w;
+ 0 if v == w;
+ 1 if v > w.
+ (This function cannot return 2.)
+*/
+static int
+adjust_tp_compare(int c)
+{
+ if (PyErr_Occurred()) {
+ if (c != -1 && c != -2) {
+ PyObject *t, *v, *tb;
+ PyErr_Fetch(&t, &v, &tb);
+ if (PyErr_Warn(PyExc_RuntimeWarning,
+ "tp_compare didn't return -1 or -2 "
+ "for exception") < 0) {
+ Py_XDECREF(t);
+ Py_XDECREF(v);
+ Py_XDECREF(tb);
+ }
+ else
+ PyErr_Restore(t, v, tb);
+ }
+ return -2;
+ }
+ else if (c < -1 || c > 1) {
+ if (PyErr_Warn(PyExc_RuntimeWarning,
+ "tp_compare didn't return -1, 0 or 1") < 0)
+ return -2;
+ else
+ return c < -1 ? -1 : 1;
+ }
+ else {
+ assert(c >= -1 && c <= 1);
+ return c;
+ }
+}
+
+
/* Macro to get the tp_richcompare field of a type if defined */
#define RICHCOMPARE(t) (PyType_HasFeature((t), Py_TPFLAGS_HAVE_RICHCOMPARE) \
? (t)->tp_richcompare : NULL)
@@ -480,9 +520,7 @@
/* If both have the same (non-NULL) tp_compare, use it. */
if (f != NULL && f == w->ob_type->tp_compare) {
c = (*f)(v, w);
- if (c < 0 && PyErr_Occurred())
- return -1;
- return c < 0 ? -1 : c > 0 ? 1 : 0;
+ return adjust_tp_compare(c);
}
/* If either tp_compare is _PyObject_SlotCompare, that's safe. */
@@ -502,9 +540,7 @@
c = (*f)(v, w);
Py_DECREF(v);
Py_DECREF(w);
- if (c < 0 && PyErr_Occurred())
- return -2;
- return c < 0 ? -1 : c > 0 ? 1 : 0;
+ return adjust_tp_compare(c);
}
/* Try w's comparison, if defined */
@@ -512,9 +548,11 @@
c = (*f)(w, v); /* swapped! */
Py_DECREF(v);
Py_DECREF(w);
- if (c < 0 && PyErr_Occurred())
- return -2;
- return c < 0 ? 1 : c > 0 ? -1 : 0; /* negated! */
+ c = adjust_tp_compare(c);
+ if (c >= -1)
+ return -c; /* Swapped! */
+ else
+ return c;
}
/* No comparison defined */
@@ -591,11 +629,11 @@
#define CHECK_TYPES(o) PyType_HasFeature((o)->ob_type, Py_TPFLAGS_CHECKTYPES)
/* Do a 3-way comparison, by hook or by crook. Return:
- -2 for an exception;
+ -2 for an exception (but see below);
-1 if v < w;
0 if v == w;
1 if v > w;
- If the object implements a tp_compare function, it returns
+ BUT: if the object implements a tp_compare function, it returns
whatever this function returns (whether with an exception or not).
*/
static int
@@ -607,9 +645,22 @@
if (v->ob_type == w->ob_type
&& (f = v->ob_type->tp_compare) != NULL) {
c = (*f)(v, w);
- if (c != 2 || !PyInstance_Check(v))
- return c;
+ if (PyInstance_Check(v)) {
+ /* Instance tp_compare has a different signature.
+ But if it returns undefined we fall through. */
+ if (c != 2)
+ return c;
+ /* Else fall throug to try_rich_to_3way_compare() */
+ }
+ else
+ return adjust_tp_compare(c);
}
+ /* We only get here if one of the following is true:
+ a) v and w have different types
+ b) v and w have the same type, which doesn't have tp_compare
+ c) v and w are instances, and either __cmp__ is not defined or
+ __cmp__ returns NotImplemented
+ */
c = try_rich_to_3way_compare(v, w);
if (c < 2)
return c;
@@ -902,7 +953,8 @@
fcmp = v->ob_type->tp_compare;
if (fcmp != NULL) {
int c = (*fcmp)(v, w);
- if (c < 0 && PyErr_Occurred()) {
+ c = adjust_tp_compare(c);
+ if (c == -2) {
res = NULL;
goto Done;
}