Opened 4 years ago
Closed 4 years ago
#24930 closed enhancement (fixed)
py3: some cmp removal in pyx files
Reported by: | chapoton | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-8.2 |
Component: | python3 | Keywords: | python3, richcmp |
Cc: | Merged in: | ||
Authors: | Frédéric Chapoton | Reviewers: | Travis Scrimshaw, Jeroen Demeyer |
Report Upstream: | N/A | Work issues: | |
Branch: | 47c7694 (Commits, GitHub, GitLab) | Commit: | 47c7694486866fad2c7bebd7127a7239864a6b04 |
Dependencies: | Stopgaps: |
Description
part of #16537
Change History (15)
comment:1 Changed 4 years ago by
- Branch set to u/chapoton/24930
- Commit set to 6dd8b130782889c1fbc5cf46f654742e4304bf61
- Status changed from new to needs_review
comment:2 Changed 4 years ago by
- Cc python3 richcmp added
comment:3 Changed 4 years ago by
- Cc python3 richcmp removed
- Keywords python3 richcmp added
comment:4 Changed 4 years ago by
- Reviewers set to Travis Scrimshaw
- Status changed from needs_review to positive_review
LGTM.
comment:5 Changed 4 years ago by
- Status changed from positive_review to needs_work
comment:6 Changed 4 years ago by
- In
__richcmp__
, you really should deal with the case of different classes. Typically, this is done by addingif not isinstance(other, TheClassThatYouWant): return NotImplemented
Otherwise, things might break sooner or later (see #19628 for an example of such breakage).
- Once you did check the class with
isinstance()
, you can do a cast. So replacecdef TheClassThatYouWant x = other
by
x = <TheClassThatYouWant>other
(you can still add cdef TheClassThatYouWant
but that is redundant).
- 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.
comment:7 Changed 4 years ago by
- I believe that the code in
intlist.pyx
is actually wrong. The second argument torich_to_bool()
must be -1, 0 or 1. If you want to support other integers, you needrich_to_bool_sgn()
.
comment:8 Changed 4 years ago by
FWIW this was my fix for intlist from my Python 3 branch:
-
src/sage/stats/intlist.pyx
diff --git a/src/sage/stats/intlist.pyx b/src/sage/stats/intlist.pyx index 6076475..c6d3f8f 100644
a b from libc.string cimport memcpy 32 32 from cysignals.memory cimport sig_malloc, sig_free 33 33 from cysignals.signals cimport sig_on, sig_off 34 34 35 from sage.structure.richcmp import rich_to_bool 35 36 from sage.rings.integer import Integer 36 37 from sage.finance.time_series cimport TimeSeries 37 38 from cpython.bytes cimport PyBytes_FromStringAndSize, PyBytes_AsString … … cdef class IntList: 116 117 for i in range(self._length): 117 118 self._values[i] = values[i] 118 119 119 def __ cmp__(self, _other):120 def __richcmp__(left, right, op): 120 121 """ 121 122 Compare self and other. This has the same semantics 122 123 as list comparison. … … cdef class IntList: 133 134 sage: w == w 134 135 True 135 136 """ 136 cdef IntList other137 cdef IntList l, r 137 138 cdef Py_ssize_t c, i 138 139 cdef int d 139 if not isinstance(_other, IntList): 140 _other = IntList(_other) 141 other = _other 142 for i in range(min(self._length, other._length)): 143 d = self._values[i] - other._values[i] 144 if d: return -1 if d < 0 else 1 145 c = self._length - other._length 146 if c < 0: return -1 147 elif c > 0: return 1 148 return 0 140 if not isinstance(right, IntList): 141 right = IntList(right) 142 l, r = left, right 143 for i in range(min(l._length, r._length)): 144 d = l._values[i] - r._values[i] 145 if d: 146 return rich_to_bool(op, -1 if d < 0 else 1) 147 c = l._length - r._length 148 if c < 0: 149 return rich_to_bool(op, -1) 150 elif c > 0: 151 return rich_to_bool(op, +1) 152 return rich_to_bool(op, 0)
comment:9 Changed 4 years ago by
- Commit changed from 6dd8b130782889c1fbc5cf46f654742e4304bf61 to 47c7694486866fad2c7bebd7127a7239864a6b04
Branch pushed to git repo; I updated commit sha1. New commits:
47c7694 | trac 24930 enhancements, corrections
|
comment:10 Changed 4 years ago by
- Status changed from needs_work to needs_review
comment:11 Changed 4 years ago by
There is an rc of Cython 0.28 (#24111), so you might as well wait for that.
comment:12 Changed 4 years ago by
Well, time is flying, and there is no really good reason to wait..
comment:13 Changed 4 years ago by
- Reviewers changed from Travis Scrimshaw to Travis Scrimshaw, Jeroen Demeyer
- Status changed from needs_review to positive_review
comment:14 Changed 4 years ago by
It looks good to me--we can revisit some of the __richcmp__ -> __eq__
rewrites once we can fully depend on Cython 0.28.
comment:15 Changed 4 years ago by
- Branch changed from u/chapoton/24930 to 47c7694486866fad2c7bebd7127a7239864a6b04
- Resolution set to fixed
- Status changed from positive_review to closed
New commits:
some __cmp__ removal in pyx files