Opened 7 years ago
Closed 7 years ago
#18322 closed enhancement (fixed)
_cmp should try _richcmp_ if _cmp_ failed
Reported by: | jdemeyer | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-6.7 |
Component: | coercion | Keywords: | |
Cc: | vdelecroix | Merged in: | |
Authors: | Jeroen Demeyer | Reviewers: | Vincent Delecroix |
Report Upstream: | N/A | Work issues: | |
Branch: | ac9ca1c (Commits, GitHub, GitLab) | Commit: | ac9ca1c83737253b380a79cde33b4908a51d0ead |
Dependencies: | Stopgaps: |
Description
With the current comparison implementation, defining __cmp__
in a Cython class can actually break cmp()
if _cmp_
raises NotImplementedError
. This is because defining __cmp__
causes Python to use __cmp__
and only __cmp__
. Instead, __cmp__
(so really _cmp
in Cython) should itself try _richcmp_
if _cmp_
didn't work.
To avoid an infinite loop, it's important that only _cmp
calls _richcmp_
, we should not change _cmp_
here.
Change History (14)
comment:1 Changed 7 years ago by
- Branch set to u/jdemeyer/_cmp_should_try__richcmp__if__cmp__failed
comment:2 Changed 7 years ago by
- Commit set to c1d1881c5034a6712b2efc2c735788494aefbeeb
- Status changed from new to needs_review
comment:3 Changed 7 years ago by
- Commit changed from c1d1881c5034a6712b2efc2c735788494aefbeeb to c70bd3973cc30346d965f3cee6db2648d081be80
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
c70bd39 | _cmp should try _richcmp_ if _cmp_ failed
|
comment:4 Changed 7 years ago by
- Commit changed from c70bd3973cc30346d965f3cee6db2648d081be80 to 621bd1020cadde224d361c0711ddb2ceba2378ed
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
621bd10 | _cmp should try _richcmp_ if _cmp_ failed
|
comment:5 Changed 7 years ago by
- Dependencies #17890 deleted
comment:6 Changed 7 years ago by
- Commit changed from 621bd1020cadde224d361c0711ddb2ceba2378ed to cb07254a1a8ec6b862a42492ddfecd0cb238d4e0
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
cb07254 | _cmp should try _richcmp_ if _cmp_ failed
|
comment:7 Changed 7 years ago by
- Commit changed from cb07254a1a8ec6b862a42492ddfecd0cb238d4e0 to bb98fd07ee04b18a4ec329aeb2bf061dab4f7d6d
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
bb98fd0 | _cmp should try _richcmp_ if _cmp_ failed
|
comment:8 follow-up: ↓ 10 Changed 7 years ago by
- Status changed from needs_review to needs_work
- The code of
_cmp_
raises aNotImplementedError
which has a custom error message which involves string formatting. As this error is catched in a critical part of the code (i.e._cmp
) I would either:- remove the message
"comparison not implemented for %r"%type(left)
- or use lazy strings
- remove the message
The second option looks better to me.
- It would be nice to update this comment:
#################################################################### # For a derived Cython class, you **must** put the __richcmp__ ... # arguments have identical parents. ####################################################################
with something like:implementing `_richcmp_` will automatically makes `cmp` works, see the implementation of Element._cmp
.
- Actually, the current implementation let met think that for elements we should:
- implement
_richcmp_
for more speed (e.g.==
is cheap but<
is not) or if_cmp_
does not make sense (e.g. we want to test for equality but raise errors on<
) - implement
_cmp_
otherwise
- implement
Am I right?
Vincent
comment:9 Changed 7 years ago by
- Commit changed from bb98fd07ee04b18a4ec329aeb2bf061dab4f7d6d to d826520e60f02169ff9b93e2e6c29eb662d1d14f
Branch pushed to git repo; I updated commit sha1. New commits:
d826520 | Use LazyFormat for _cmp_ exception
|
comment:10 in reply to: ↑ 8 Changed 7 years ago by
Replying to vdelecroix:
- Actually, the current implementation let met think that for elements we should:
- implement
_richcmp_
for more speed (e.g.==
is cheap but<
is not) or if_cmp_
does not make sense (e.g. we want to test for equality but raise errors on<
)- implement
_cmp_
otherwise
I think so, yes.
comment:11 Changed 7 years ago by
- Commit changed from d826520e60f02169ff9b93e2e6c29eb662d1d14f to ac9ca1c83737253b380a79cde33b4908a51d0ead
Branch pushed to git repo; I updated commit sha1. New commits:
ac9ca1c | Update comment
|
comment:12 Changed 7 years ago by
- Reviewers set to Vincent Delecroix
- Status changed from needs_work to needs_review
comment:13 Changed 7 years ago by
- Status changed from needs_review to positive_review
comment:14 Changed 7 years ago by
- Branch changed from u/jdemeyer/_cmp_should_try__richcmp__if__cmp__failed to ac9ca1c83737253b380a79cde33b4908a51d0ead
- Resolution set to fixed
- Status changed from positive_review to closed
Last 10 new commits:
Implement _rich_to_bool as inline function instead of member function
Merge tag '6.7.beta2' into t/17890/ticket/17890
Optimize rich_to_bool_sgn
Improve comparisons for permutation groups
Improve _richcmp and documentation
Fix doctest formatting
Fix bad doctest in etaproducts
Add pointers for special uses of __richcmp__
Merge commit '3976f2cc4ec59c9f978d9d02c7f697c81025c4ef' into t/18322/_cmp_should_try__richcmp__if__cmp__failed
_cmp should try _richcmp_ if _cmp_ failed