bpo-23699: Use a macro to reduce boilerplate code in rich comparison functions (GH-793)

Doc/c-api/typeobj.rst

@@ -623,6 +623,22 @@ type objects) *must* have the :attr:`ob_size` field.
    | :const:`Py_GE` | ``>=``     |
    +----------------+------------+
 
+   The following macro is defined to ease writing rich comparison functions:
+
+   .. c:function:: PyObject *Py_RETURN_RICHCOMPARE(VAL_A, VAL_B, int op)
+
+      Return ``Py_True`` or ``Py_False`` from the function, depending on the
+      result of a comparison.
+      VAL_A and VAL_B must be orderable by C comparison operators (for example,
+      they may be C ints or floats). The third argument specifies the requested
+      operation, as for :c:func:`PyObject_RichCompare`.
+
+      The return value's reference count is properly incremented.
+
+      On error, sets an exception and returns NULL from the function.
+
+      .. versionadded:: 3.7
+
 
 .. c:member:: Py_ssize_t PyTypeObject.tp_weaklistoffset
 

Include/object.h

@@ -931,6 +931,25 @@ PyAPI_DATA(PyObject) _Py_NotImplementedStruct; /* Don't use this directly */
 #define Py_GT 4
 #define Py_GE 5
 
+/*
+ * Macro for implementing rich comparisons
+ *
+ * Needs to be a macro because any C-comparable type can be used.
+ */
+#define Py_RETURN_RICHCOMPARE(val1, val2, op)                               \
+    do {                                                                    \
+        switch (op) {                                                       \
+        case Py_EQ: if ((val1) == (val2)) Py_RETURN_TRUE; Py_RETURN_FALSE;  \
+        case Py_NE: if ((val1) != (val2)) Py_RETURN_TRUE; Py_RETURN_FALSE;  \
+        case Py_LT: if ((val1) < (val2)) Py_RETURN_TRUE; Py_RETURN_FALSE;   \
+        case Py_GT: if ((val1) > (val2)) Py_RETURN_TRUE; Py_RETURN_FALSE;   \
+        case Py_LE: if ((val1) <= (val2)) Py_RETURN_TRUE; Py_RETURN_FALSE;  \
+        case Py_GE: if ((val1) >= (val2)) Py_RETURN_TRUE; Py_RETURN_FALSE;  \
+        default:                                                            \
+            Py_UNREACHABLE();                                               \
+        }                                                                   \
+    } while (0)
+
 #ifndef Py_LIMITED_API
 /* Maps Py_LT to Py_GT, ..., Py_GE to Py_LE.
  * Defined in object.c.

Misc/ACKS

@@ -1646,7 +1646,7 @@ Mike Verdone
 Jaap Vermeulen
 Nikita Vetoshkin
 Al Vezza
-Petr Victorin
+Petr Viktorin
 Jacques A. Vidrine
 John Viega
 Dino Viehland

Misc/NEWS.d/next/C API/2017-10-19-15-27-04.bpo-23699.-noVVc.rst

@@ -0,0 +1,2 @@
+Add Py_RETURN_RICHCOMPARE macro to reduce boilerplate code in rich
+comparison functions.

Modules/_datetimemodule.c

@@ -1442,22 +1442,7 @@ build_struct_time(int y, int m, int d, int hh, int mm, int ss, int dstflag)
 static PyObject *
 diff_to_bool(int diff, int op)
 {
-    PyObject *result;
+    Py_RETURN_RICHCOMPARE(diff, 0, op);
-    int istrue;
-
-    switch (op) {
-        case Py_EQ: istrue = diff == 0; break;
-        case Py_NE: istrue = diff != 0; break;
-        case Py_LE: istrue = diff <= 0; break;
-        case Py_GE: istrue = diff >= 0; break;
-        case Py_LT: istrue = diff < 0; break;
-        case Py_GT: istrue = diff > 0; break;
-        default:
-            Py_UNREACHABLE();
-    }
-    result = istrue ? Py_True : Py_False;
-    Py_INCREF(result);
-    return result;
 }
 
 /* Raises a "can't compare" TypeError and returns NULL. */

Modules/_tkinter.c

@@ -864,13 +864,10 @@ PyTclObject_repr(PyTclObject *self)
     return repr;
 }
 
-#define TEST_COND(cond) ((cond) ? Py_True : Py_False)
-
 static PyObject *
 PyTclObject_richcompare(PyObject *self, PyObject *other, int op)
 {
     int result;
-    PyObject *v;
 
     /* neither argument should be NULL, unless something's gone wrong */
     if (self == NULL || other == NULL) {
@@ -880,8 +877,7 @@ PyTclObject_richcompare(PyObject *self, PyObject *other, int op)
 
     /* both arguments should be instances of PyTclObject */
     if (!PyTclObject_Check(self) || !PyTclObject_Check(other)) {
-        v = Py_NotImplemented;
+        Py_RETURN_NOTIMPLEMENTED;
-        goto finished;
     }
 
     if (self == other)
@@ -890,33 +886,7 @@ PyTclObject_richcompare(PyObject *self, PyObject *other, int op)
     else
         result = strcmp(Tcl_GetString(((PyTclObject *)self)->value),
                         Tcl_GetString(((PyTclObject *)other)->value));
-    /* Convert return value to a Boolean */
+    Py_RETURN_RICHCOMPARE(result, 0, op);
-    switch (op) {
-    case Py_EQ:
-        v = TEST_COND(result == 0);
-        break;
-    case Py_NE:
-        v = TEST_COND(result != 0);
-        break;
-    case Py_LE:
-        v = TEST_COND(result <= 0);
-        break;
-    case Py_GE:
-        v = TEST_COND(result >= 0);
-        break;
-    case Py_LT:
-        v = TEST_COND(result < 0);
-        break;
-    case Py_GT:
-        v = TEST_COND(result > 0);
-        break;
-    default:
-        PyErr_BadArgument();
-        return NULL;
-    }
-  finished:
-    Py_INCREF(v);
-    return v;
 }
 
 PyDoc_STRVAR(get_typename__doc__, "name of the Tcl type");

Modules/parsermodule.c

@@ -299,13 +299,10 @@ parser_compare_nodes(node *left, node *right)
  *
  */
 
-#define TEST_COND(cond) ((cond) ? Py_True : Py_False)
-
 static PyObject *
 parser_richcompare(PyObject *left, PyObject *right, int op)
 {
     int result;
-    PyObject *v;
 
     /* neither argument should be NULL, unless something's gone wrong */
     if (left == NULL || right == NULL) {
@@ -315,8 +312,7 @@ parser_richcompare(PyObject *left, PyObject *right, int op)
 
     /* both arguments should be instances of PyST_Object */
     if (!PyST_Object_Check(left) || !PyST_Object_Check(right)) {
-        v = Py_NotImplemented;
+        Py_RETURN_NOTIMPLEMENTED;
-        goto finished;
     }
 
     if (left == right)
@@ -326,33 +322,7 @@ parser_richcompare(PyObject *left, PyObject *right, int op)
         result = parser_compare_nodes(((PyST_Object *)left)->st_node,
                                       ((PyST_Object *)right)->st_node);
 
-    /* Convert return value to a Boolean */
+    Py_RETURN_RICHCOMPARE(result, 0, op);
-    switch (op) {
-      case Py_EQ:
-        v = TEST_COND(result == 0);
-        break;
-      case Py_NE:
-        v = TEST_COND(result != 0);
-        break;
-      case Py_LE:
-        v = TEST_COND(result <= 0);
-        break;
-      case Py_GE:
-        v = TEST_COND(result >= 0);
-        break;
-      case Py_LT:
-        v = TEST_COND(result < 0);
-        break;
-      case Py_GT:
-        v = TEST_COND(result > 0);
-        break;
-      default:
-        PyErr_BadArgument();
-        return NULL;
-    }
-  finished:
-    Py_INCREF(v);
-    return v;
 }
 
 /*  parser_newstobject(node* st)

Modules/selectmodule.c

@@ -1929,27 +1929,7 @@ kqueue_event_richcompare(kqueue_event_Object *s, kqueue_event_Object *o,
            : 0;
 #undef CMP
 
-    switch (op) {
+    Py_RETURN_RICHCOMPARE(result, 0, op);
-    case Py_EQ:
-        result = (result == 0);
-        break;
-    case Py_NE:
-        result = (result != 0);
-        break;
-    case Py_LE:
-        result = (result <= 0);
-        break;
-    case Py_GE:
-        result = (result >= 0);
-        break;
-    case Py_LT:
-        result = (result < 0);
-        break;
-    case Py_GT:
-        result = (result > 0);
-        break;
-    }
-    return PyBool_FromLong((long)result);
 }
 
 static PyTypeObject kqueue_event_Type = {

Objects/bytearrayobject.c

@@ -1012,8 +1012,6 @@ bytearray_richcompare(PyObject *self, PyObject *other, int op)
 {
     Py_ssize_t self_size, other_size;
     Py_buffer self_bytes, other_bytes;
-    PyObject *res;
-    Py_ssize_t minsize;
     int cmp, rc;
 
     /* Bytes can be compared to anything that supports the (binary)
@@ -1049,38 +1047,25 @@ bytearray_richcompare(PyObject *self, PyObject *other, int op)
 
     if (self_size != other_size && (op == Py_EQ || op == Py_NE)) {
         /* Shortcut: if the lengths differ, the objects differ */
-        cmp = (op == Py_NE);
+        PyBuffer_Release(&self_bytes);
+        PyBuffer_Release(&other_bytes);
+        return PyBool_FromLong((op == Py_NE));
     }
     else {
-        minsize = self_size;
+        cmp = memcmp(self_bytes.buf, other_bytes.buf,
-        if (other_size < minsize)
+                     Py_MIN(self_size, other_size));
-            minsize = other_size;
-
-        cmp = memcmp(self_bytes.buf, other_bytes.buf, minsize);
         /* In ISO C, memcmp() guarantees to use unsigned bytes! */
 
-        if (cmp == 0) {
+        PyBuffer_Release(&self_bytes);
-            if (self_size < other_size)
+        PyBuffer_Release(&other_bytes);
-                cmp = -1;
+
-            else if (self_size > other_size)
+        if (cmp != 0) {
-                cmp = 1;
+            Py_RETURN_RICHCOMPARE(cmp, 0, op);
         }
 
-        switch (op) {
+        Py_RETURN_RICHCOMPARE(self_size, other_size, op);
-        case Py_LT: cmp = cmp <  0; break;
-        case Py_LE: cmp = cmp <= 0; break;
-        case Py_EQ: cmp = cmp == 0; break;
-        case Py_NE: cmp = cmp != 0; break;
-        case Py_GT: cmp = cmp >  0; break;
-        case Py_GE: cmp = cmp >= 0; break;
-        }
     }
 
-    res = cmp ? Py_True : Py_False;
-    PyBuffer_Release(&self_bytes);
-    PyBuffer_Release(&other_bytes);
-    Py_INCREF(res);
-    return res;
 }
 
 static void

Objects/bytesobject.c

@@ -1566,7 +1566,6 @@ bytes_richcompare(PyBytesObject *a, PyBytesObject *b, int op)
     int c;
     Py_ssize_t len_a, len_b;
     Py_ssize_t min_len;
-    PyObject *result;
     int rc;
 
     /* Make sure both arguments are strings. */
@@ -1599,7 +1598,7 @@ bytes_richcompare(PyBytesObject *a, PyBytesObject *b, int op)
                 }
             }
         }
-        result = Py_NotImplemented;
+        Py_RETURN_NOTIMPLEMENTED;
     }
     else if (a == b) {
         switch (op) {
@@ -1607,12 +1606,12 @@ bytes_richcompare(PyBytesObject *a, PyBytesObject *b, int op)
         case Py_LE:
         case Py_GE:
             /* a string is equal to itself */
-            result = Py_True;
+            Py_RETURN_TRUE;
             break;
         case Py_NE:
         case Py_LT:
         case Py_GT:
-            result = Py_False;
+            Py_RETURN_FALSE;
             break;
         default:
             PyErr_BadArgument();
@@ -1622,7 +1621,7 @@ bytes_richcompare(PyBytesObject *a, PyBytesObject *b, int op)
     else if (op == Py_EQ || op == Py_NE) {
         int eq = bytes_compare_eq(a, b);
         eq ^= (op == Py_NE);
-        result = eq ? Py_True : Py_False;
+        return PyBool_FromLong(eq);
     }
     else {
         len_a = Py_SIZE(a);
@@ -1635,22 +1634,10 @@ bytes_richcompare(PyBytesObject *a, PyBytesObject *b, int op)
         }
         else
             c = 0;
-        if (c == 0)
+        if (c != 0)
-            c = (len_a < len_b) ? -1 : (len_a > len_b) ? 1 : 0;
+            Py_RETURN_RICHCOMPARE(c, 0, op);
-        switch (op) {
+        Py_RETURN_RICHCOMPARE(len_a, len_b, op);
-        case Py_LT: c = c <  0; break;
-        case Py_LE: c = c <= 0; break;
-        case Py_GT: c = c >  0; break;
-        case Py_GE: c = c >= 0; break;
-        default:
-            PyErr_BadArgument();
-            return NULL;
-        }
-        result = c ? Py_True : Py_False;
     }
-
-    Py_INCREF(result);
-    return result;
 }
 
 static Py_hash_t

Objects/cellobject.c

@@ -53,22 +53,15 @@ cell_dealloc(PyCellObject *op)
     PyObject_GC_Del(op);
 }
 
-#define TEST_COND(cond) ((cond) ? Py_True : Py_False)
-
 static PyObject *
 cell_richcompare(PyObject *a, PyObject *b, int op)
 {
-    int result;
-    PyObject *v;
-
     /* neither argument should be NULL, unless something's gone wrong */
     assert(a != NULL && b != NULL);
 
     /* both arguments should be instances of PyCellObject */
     if (!PyCell_Check(a) || !PyCell_Check(b)) {
-        v = Py_NotImplemented;
+        Py_RETURN_NOTIMPLEMENTED;
-        Py_INCREF(v);
-        return v;
     }
 
     /* compare cells by contents; empty cells come before anything else */
@@ -77,32 +70,7 @@ cell_richcompare(PyObject *a, PyObject *b, int op)
     if (a != NULL && b != NULL)
         return PyObject_RichCompare(a, b, op);
 
-    result = (b == NULL) - (a == NULL);
+    Py_RETURN_RICHCOMPARE(b == NULL, a == NULL, op);
-    switch (op) {
-    case Py_EQ:
-        v = TEST_COND(result == 0);
-        break;
-    case Py_NE:
-        v = TEST_COND(result != 0);
-        break;
-    case Py_LE:
-        v = TEST_COND(result <= 0);
-        break;
-    case Py_GE:
-        v = TEST_COND(result >= 0);
-        break;
-    case Py_LT:
-        v = TEST_COND(result < 0);
-        break;
-    case Py_GT:
-        v = TEST_COND(result > 0);
-        break;
-    default:
-        PyErr_BadArgument();
-        return NULL;
-    }
-    Py_INCREF(v);
-    return v;
 }
 
 static PyObject *

Objects/descrobject.c

@@ -1035,22 +1035,16 @@ wrapper_dealloc(wrapperobject *wp)
     Py_TRASHCAN_SAFE_END(wp)
 }
 
-#define TEST_COND(cond) ((cond) ? Py_True : Py_False)
-
 static PyObject *
 wrapper_richcompare(PyObject *a, PyObject *b, int op)
 {
-    intptr_t result;
-    PyObject *v;
     PyWrapperDescrObject *a_descr, *b_descr;
 
     assert(a != NULL && b != NULL);
 
     /* both arguments should be wrapperobjects */
     if (!Wrapper_Check(a) || !Wrapper_Check(b)) {
-        v = Py_NotImplemented;
+        Py_RETURN_NOTIMPLEMENTED;
-        Py_INCREF(v);
-        return v;
     }
 
     /* compare by descriptor address; if the descriptors are the same,
@@ -1063,32 +1057,7 @@ wrapper_richcompare(PyObject *a, PyObject *b, int op)
         return PyObject_RichCompare(a, b, op);
     }
 
-    result = a_descr - b_descr;
+    Py_RETURN_RICHCOMPARE(a_descr, b_descr, op);
-    switch (op) {
-    case Py_EQ:
-        v = TEST_COND(result == 0);
-        break;
-    case Py_NE:
-        v = TEST_COND(result != 0);
-        break;
-    case Py_LE:
-        v = TEST_COND(result <= 0);
-        break;
-    case Py_GE:
-        v = TEST_COND(result >= 0);
-        break;
-    case Py_LT:
-        v = TEST_COND(result < 0);
-        break;
-    case Py_GT:
-        v = TEST_COND(result > 0);
-        break;
-    default:
-        PyErr_BadArgument();
-        return NULL;
-    }
-    Py_INCREF(v);
-    return v;
 }
 
 static Py_hash_t

Objects/listobject.c

@@ -2330,13 +2330,10 @@ list_richcompare(PyObject *v, PyObject *w, int op)
 
     if (Py_SIZE(vl) != Py_SIZE(wl) && (op == Py_EQ || op == Py_NE)) {
         /* Shortcut: if the lengths differ, the lists differ */
-        PyObject *res;
         if (op == Py_EQ)
-            res = Py_False;
+            Py_RETURN_FALSE;
         else
-            res = Py_True;
+            Py_RETURN_TRUE;
-        Py_INCREF(res);
-        return res;
     }
 
     /* Search for the first index where items are different */
@@ -2351,25 +2348,7 @@ list_richcompare(PyObject *v, PyObject *w, int op)
 
     if (i >= Py_SIZE(vl) || i >= Py_SIZE(wl)) {
         /* No more items to compare -- compare sizes */
-        Py_ssize_t vs = Py_SIZE(vl);
+        Py_RETURN_RICHCOMPARE(Py_SIZE(vl), Py_SIZE(wl), op);
-        Py_ssize_t ws = Py_SIZE(wl);
-        int cmp;
-        PyObject *res;
-        switch (op) {
-        case Py_LT: cmp = vs <  ws; break;
-        case Py_LE: cmp = vs <= ws; break;
-        case Py_EQ: cmp = vs == ws; break;
-        case Py_NE: cmp = vs != ws; break;
-        case Py_GT: cmp = vs >  ws; break;
-        case Py_GE: cmp = vs >= ws; break;
-        default: return NULL; /* cannot happen */
-        }
-        if (cmp)
-            res = Py_True;
-        else
-            res = Py_False;
-        Py_INCREF(res);
-        return res;
     }
 
     /* We have an item that differs -- shortcuts for EQ/NE */

Objects/longobject.c

@@ -2915,45 +2915,16 @@ long_compare(PyLongObject *a, PyLongObject *b)
     return sign < 0 ? -1 : sign > 0 ? 1 : 0;
 }
 
-#define TEST_COND(cond) \
-    ((cond) ? Py_True : Py_False)
-
 static PyObject *
 long_richcompare(PyObject *self, PyObject *other, int op)
 {
     int result;
-    PyObject *v;
     CHECK_BINOP(self, other);
     if (self == other)
         result = 0;
     else
         result = long_compare((PyLongObject*)self, (PyLongObject*)other);
-    /* Convert the return value to a Boolean */
+    Py_RETURN_RICHCOMPARE(result, 0, op);
-    switch (op) {
-    case Py_EQ:
-        v = TEST_COND(result == 0);
-        break;
-    case Py_NE:
-        v = TEST_COND(result != 0);
-        break;
-    case Py_LE:
-        v = TEST_COND(result <= 0);
-        break;
-    case Py_GE:
-        v = TEST_COND(result >= 0);
-        break;
-    case Py_LT:
-        v = TEST_COND(result == -1);
-        break;
-    case Py_GT:
-        v = TEST_COND(result == 1);
-        break;
-    default:
-        PyErr_BadArgument();
-        return NULL;
-    }
-    Py_INCREF(v);
-    return v;
 }
 
 static Py_hash_t

Objects/tupleobject.c

@@ -647,23 +647,7 @@ tuplerichcompare(PyObject *v, PyObject *w, int op)
 
     if (i >= vlen || i >= wlen) {
         /* No more items to compare -- compare sizes */
-        int cmp;
+        Py_RETURN_RICHCOMPARE(vlen, wlen, op);
-        PyObject *res;
-        switch (op) {
-        case Py_LT: cmp = vlen <  wlen; break;
-        case Py_LE: cmp = vlen <= wlen; break;
-        case Py_EQ: cmp = vlen == wlen; break;
-        case Py_NE: cmp = vlen != wlen; break;
-        case Py_GT: cmp = vlen >  wlen; break;
-        case Py_GE: cmp = vlen >= wlen; break;
-        default: return NULL; /* cannot happen */
-        }
-        if (cmp)
-            res = Py_True;
-        else
-            res = Py_False;
-        Py_INCREF(res);
-        return res;
     }
 
     /* We have an item that differs -- shortcuts for EQ/NE */

Objects/unicodeobject.c

@@ -11152,14 +11152,10 @@ _PyUnicode_EqualToASCIIId(PyObject *left, _Py_Identifier *right)
     return unicode_compare_eq(left, right_uni);
 }
 
-#define TEST_COND(cond)                         \
-    ((cond) ? Py_True : Py_False)
-
 PyObject *
 PyUnicode_RichCompare(PyObject *left, PyObject *right, int op)
 {
     int result;
-    PyObject *v;
 
     if (!PyUnicode_Check(left) || !PyUnicode_Check(right))
         Py_RETURN_NOTIMPLEMENTED;
@@ -11174,13 +11170,11 @@ PyUnicode_RichCompare(PyObject *left, PyObject *right, int op)
         case Py_LE:
         case Py_GE:
             /* a string is equal to itself */
-            v = Py_True;
+            Py_RETURN_TRUE;
-            break;
         case Py_NE:
         case Py_LT:
         case Py_GT:
-            v = Py_False;
+            Py_RETURN_FALSE;
-            break;
         default:
             PyErr_BadArgument();
             return NULL;
@@ -11189,32 +11183,12 @@ PyUnicode_RichCompare(PyObject *left, PyObject *right, int op)
     else if (op == Py_EQ || op == Py_NE) {
         result = unicode_compare_eq(left, right);
         result ^= (op == Py_NE);
-        v = TEST_COND(result);
+        return PyBool_FromLong(result);
     }
     else {
         result = unicode_compare(left, right);
-
+        Py_RETURN_RICHCOMPARE(result, 0, op);
-        /* Convert the return value to a Boolean */
-        switch (op) {
-        case Py_LE:
-            v = TEST_COND(result <= 0);
-            break;
-        case Py_GE:
-            v = TEST_COND(result >= 0);
-            break;
-        case Py_LT:
-            v = TEST_COND(result == -1);
-            break;
-        case Py_GT:
-            v = TEST_COND(result == 1);
-            break;
-        default:
-            PyErr_BadArgument();
-            return NULL;
-        }
     }
-    Py_INCREF(v);
-    return v;
 }
 
 int