Opened 3 years ago

Closed 3 years ago

#22019 closed enhancement (fixed)

py3 remove cmp() in two pyx files in rings

Reported by: chapoton Owned by:
Priority: major Milestone: sage-7.6
Component: python3 Keywords:
Cc: tscrim, aapitzsch, jdemeyer Merged in:
Authors: Frédéric Chapoton Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: a943199 (Commits) Commit: a9431991dc62f31aa5712d081400b69e904572af
Dependencies: Stopgaps:

Description

where _richcmp calls _cmp_

Change History (12)

comment:1 Changed 3 years ago by chapoton

  • Branch set to u/chapoton/22019
  • Cc tscrim aapitzsch jdemeyer added
  • Commit set to a9431991dc62f31aa5712d081400b69e904572af
  • Status changed from new to needs_review

New commits:

a943199py3 romve cmp() in two other pyx files in rings

comment:2 Changed 3 years ago by chapoton

bot is morally green, please review

comment:3 Changed 3 years ago by chapoton

*ping*

comment:4 Changed 3 years ago by jdemeyer

Why don't you change _cmp_ to _richcmp_ here?

comment:5 Changed 3 years ago by chapoton

Because here (like in #22013), _cmp_ is called indirectly by _richcmp.

And also because my aim is to get rid of cmp(), not necessarily to change _cmp_ to _richcmp_

comment:6 Changed 3 years ago by chapoton

Could we please get try to move on here, and in the similar #22013, so that we can then try to attack the question of _cmp_ and _richcmp inside Parent ?

See also #22029

comment:7 follow-up: Changed 3 years ago by tscrim

Instead of an arbitrary -1, how about a NotImplemented as in #21971? This is at least a valid hack until we move to _richcmp_.

comment:8 in reply to: ↑ 7 Changed 3 years ago by chapoton

Replying to tscrim:

Instead of an arbitrary -1, how about a NotImplemented as in #21971? This is at least a valid hack until we move to _richcmp_.

Hmm, maybe the NotImplemented? in #21971 was an error. Anyway, here it is not allowed by the type of the _cmp_function, so arbitrary -1 is the only option.

comment:9 Changed 3 years ago by chapoton

  • Milestone changed from sage-7.5 to sage-7.6

comment:10 Changed 3 years ago by chapoton

I would like this one to go in also, even if it is only a partial progress.

Recall that my current aim is to cythonize every pyx file in python3, and for that one has to remove all cmp() in pyx files.

comment:11 Changed 3 years ago by tscrim

  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to positive_review

This will work for now. Since these are parents, they probably should just use __richcmp__ directly, but that can be an issue for another day.

comment:12 Changed 3 years ago by vbraun

  • Branch changed from u/chapoton/22019 to a9431991dc62f31aa5712d081400b69e904572af
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.