From e8b19656396381407ad91473af5da8b0d4346e88 Mon Sep 17 00:00:00 2001 From: stratakis Date: Thu, 2 Nov 2017 11:32:54 +0100 Subject: bpo-23699: Use a macro to reduce boilerplate code in rich comparison functions (GH-793) --- Doc/c-api/typeobj.rst | 16 ++++++++++ Include/object.h | 19 +++++++++++ Misc/ACKS | 2 +- .../C API/2017-10-19-15-27-04.bpo-23699.-noVVc.rst | 2 ++ Modules/_datetimemodule.c | 17 +--------- Modules/_tkinter.c | 34 ++------------------ Modules/parsermodule.c | 34 ++------------------ Modules/selectmodule.c | 22 +------------ Objects/bytearrayobject.c | 37 +++++++--------------- Objects/bytesobject.c | 27 ++++------------ Objects/cellobject.c | 36 ++------------------- Objects/descrobject.c | 35 ++------------------ Objects/listobject.c | 27 ++-------------- Objects/longobject.c | 31 +----------------- Objects/tupleobject.c | 18 +---------- Objects/unicodeobject.c | 34 +++----------------- 16 files changed, 75 insertions(+), 316 deletions(-) create mode 100644 Misc/NEWS.d/next/C API/2017-10-19-15-27-04.bpo-23699.-noVVc.rst diff --git a/Doc/c-api/typeobj.rst b/Doc/c-api/typeobj.rst index 0b4577f..3bdf45a 100644 --- a/Doc/c-api/typeobj.rst +++ b/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 diff --git a/Include/object.h b/Include/object.h index cb57359..b7051f3 100644 --- a/Include/object.h +++ b/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. diff --git a/Misc/ACKS b/Misc/ACKS index ac7a258..1bd3c04 100644 --- a/Misc/ACKS +++ b/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 diff --git a/Misc/NEWS.d/next/C API/2017-10-19-15-27-04.bpo-23699.-noVVc.rst b/Misc/NEWS.d/next/C API/2017-10-19-15-27-04.bpo-23699.-noVVc.rst new file mode 100644 index 0000000..d873666 --- /dev/null +++ b/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. diff --git a/Modules/_datetimemodule.c b/Modules/_datetimemodule.c index 09b5579..b50cdda 100644 --- a/Modules/_datetimemodule.c +++ b/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; - 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; + Py_RETURN_RICHCOMPARE(diff, 0, op); } /* Raises a "can't compare" TypeError and returns NULL. */ diff --git a/Modules/_tkinter.c b/Modules/_tkinter.c index 005214e..7123da5 100644 --- a/Modules/_tkinter.c +++ b/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; - goto finished; + Py_RETURN_NOTIMPLEMENTED; } 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 */ - 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; + Py_RETURN_RICHCOMPARE(result, 0, op); } PyDoc_STRVAR(get_typename__doc__, "name of the Tcl type"); diff --git a/Modules/parsermodule.c b/Modules/parsermodule.c index 929f2de..2b98be4 100644 --- a/Modules/parsermodule.c +++ b/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; - goto finished; + Py_RETURN_NOTIMPLEMENTED; } 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 */ - 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; + Py_RETURN_RICHCOMPARE(result, 0, op); } /* parser_newstobject(node* st) diff --git a/Modules/selectmodule.c b/Modules/selectmodule.c index 1cde6e8..f2f5cc8 100644 --- a/Modules/selectmodule.c +++ b/Modules/selectmodule.c @@ -1929,27 +1929,7 @@ kqueue_event_richcompare(kqueue_event_Object *s, kqueue_event_Object *o, : 0; #undef CMP - switch (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); + Py_RETURN_RICHCOMPARE(result, 0, op); } static PyTypeObject kqueue_event_Type = { diff --git a/Objects/bytearrayobject.c b/Objects/bytearrayobject.c index c92cfc0..83c3549 100644 --- a/Objects/bytearrayobject.c +++ b/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; - if (other_size < minsize) - minsize = other_size; - - cmp = memcmp(self_bytes.buf, other_bytes.buf, minsize); + cmp = memcmp(self_bytes.buf, other_bytes.buf, + Py_MIN(self_size, other_size)); /* In ISO C, memcmp() guarantees to use unsigned bytes! */ - if (cmp == 0) { - if (self_size < other_size) - cmp = -1; - else if (self_size > other_size) - cmp = 1; - } + PyBuffer_Release(&self_bytes); + PyBuffer_Release(&other_bytes); - switch (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; + if (cmp != 0) { + Py_RETURN_RICHCOMPARE(cmp, 0, op); } + + Py_RETURN_RICHCOMPARE(self_size, other_size, op); } - res = cmp ? Py_True : Py_False; - PyBuffer_Release(&self_bytes); - PyBuffer_Release(&other_bytes); - Py_INCREF(res); - return res; } static void diff --git a/Objects/bytesobject.c b/Objects/bytesobject.c index 7ba90aa..a921d9c 100644 --- a/Objects/bytesobject.c +++ b/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) - c = (len_a < len_b) ? -1 : (len_a > len_b) ? 1 : 0; - switch (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; + if (c != 0) + Py_RETURN_RICHCOMPARE(c, 0, op); + Py_RETURN_RICHCOMPARE(len_a, len_b, op); } - - Py_INCREF(result); - return result; } static Py_hash_t diff --git a/Objects/cellobject.c b/Objects/cellobject.c index af19229..7b05e61 100644 --- a/Objects/cellobject.c +++ b/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_INCREF(v); - return v; + Py_RETURN_NOTIMPLEMENTED; } /* 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); - 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; + Py_RETURN_RICHCOMPARE(b == NULL, a == NULL, op); } static PyObject * diff --git a/Objects/descrobject.c b/Objects/descrobject.c index 5dc27ef..71d5224 100644 --- a/Objects/descrobject.c +++ b/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_INCREF(v); - return v; + Py_RETURN_NOTIMPLEMENTED; } /* 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; - 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; + Py_RETURN_RICHCOMPARE(a_descr, b_descr, op); } static Py_hash_t diff --git a/Objects/listobject.c b/Objects/listobject.c index 8585322..7eba61e 100644 --- a/Objects/listobject.c +++ b/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_INCREF(res); - return res; + Py_RETURN_TRUE; } /* 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_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; + Py_RETURN_RICHCOMPARE(Py_SIZE(vl), Py_SIZE(wl), op); } /* We have an item that differs -- shortcuts for EQ/NE */ diff --git a/Objects/longobject.c b/Objects/longobject.c index c71b783..6fd4feb 100644 --- a/Objects/longobject.c +++ b/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 */ - 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; + Py_RETURN_RICHCOMPARE(result, 0, op); } static Py_hash_t diff --git a/Objects/tupleobject.c b/Objects/tupleobject.c index c150af8..35decd8 100644 --- a/Objects/tupleobject.c +++ b/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; - 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; + Py_RETURN_RICHCOMPARE(vlen, wlen, op); } /* We have an item that differs -- shortcuts for EQ/NE */ diff --git a/Objects/unicodeobject.c b/Objects/unicodeobject.c index 6533f41..194c5bc 100644 --- a/Objects/unicodeobject.c +++ b/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; - break; + Py_RETURN_TRUE; case Py_NE: case Py_LT: case Py_GT: - v = Py_False; - break; + Py_RETURN_FALSE; 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); - - /* 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_RETURN_RICHCOMPARE(result, 0, op); } - Py_INCREF(v); - return v; } int -- cgit v0.12