py3: some cmp removal in pyx files

[chapoton]

part of #16537

[chapoton]

New commits:

​6dd8b13

some __cmp__ removal in pyx files

[tscrim]

LGTM.

[jdemeyer]

1. In __richcmp__, you really should deal with the case of different classes. Typically, this is done by adding

   if not isinstance(other, TheClassThatYouWant):
       return NotImplemented
   

Otherwise, things might break sooner or later (see #19628 for an example of such breakage).

2. Once you did check the class with isinstance(), you can do a cast. So replace

   cdef TheClassThatYouWant x = other
   

by

x = <TheClassThatYouWant>other

(you can still add cdef TheClassThatYouWant but that is redundant).

3. In the light of Cython 0.28, I would prefer to see

   def __richcmp__(TheClassThatYouWant self, other, int op)
       ...
   

instead of

def __richcmp__(self, other, int op)
    cdef TheClassThatYouWant s = self

Ideally, Cython should know that self is of type TheClassThatYouWant. But this is not the case due to a Cython bug which is fixed in version 0.28.

[jdemeyer]

4. I believe that the code in intlist.pyx is actually wrong. The second argument to rich_to_bool() must be -1, 0 or 1. If you want to support other integers, you need rich_to_bool_sgn().

[embray]

FWIW this was my fix for intlist from my Python 3 branch:

src/sage/stats/intlist.pyx

@@ -32 +32 @@
 from cysignals.memory cimport sig_malloc, sig_free
 from cysignals.signals cimport sig_on, sig_off
 
+from sage.structure.richcmp import rich_to_bool
 from sage.rings.integer import Integer
 from sage.finance.time_series cimport TimeSeries
 from cpython.bytes cimport PyBytes_FromStringAndSize, PyBytes_AsString
@@ -116 +117 @@
             for i in range(self._length):
                 self._values[i] = values[i]
 
-    def __cmp__(self, _other):
+    def __richcmp__(left, right, op):
         """
         Compare self and other.  This has the same semantics
         as list comparison.
@@ -133 +134 @@
             sage: w == w
             True
         """
-        cdef IntList other
+        cdef IntList l, r
         cdef Py_ssize_t c, i
         cdef int d
-        if not isinstance(_other, IntList):
-            _other = IntList(_other)
-        other = _other
-        for i in range(min(self._length, other._length)):
-            d = self._values[i] - other._values[i]
-            if d: return -1 if d < 0 else 1
-        c = self._length - other._length
-        if c < 0: return -1
-        elif c > 0: return 1
-        return 0
+        if not isinstance(right, IntList):
+            right = IntList(right)
+        l, r = left, right
+        for i in range(min(l._length, r._length)):
+            d = l._values[i] - r._values[i]
+            if d:
+                return rich_to_bool(op, -1 if d < 0 else 1)
+        c = l._length - r._length
+        if c < 0:
+            return rich_to_bool(op, -1)
+        elif c > 0:
+            return rich_to_bool(op, +1)
+        return rich_to_bool(op, 0)

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​47c7694

trac 24930 enhancements, corrections

[jdemeyer]

There is an rc of Cython 0.28 (#24111), so you might as well wait for that.

[chapoton]

Well, time is flying, and there is no really good reason to wait..

[embray]

It looks good to me--we can revisit some of the __richcmp__ -> __eq__ rewrites once we can fully depend on Cython 0.28.