Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement @total_ordering for cdef classes #2090

Closed
scoder opened this issue Feb 2, 2018 · 10 comments · Fixed by #4195
Closed

Implement @total_ordering for cdef classes #2090

scoder opened this issue Feb 2, 2018 · 10 comments · Fixed by #4195

Comments

@scoder
Copy link
Contributor

scoder commented Feb 2, 2018

We now support the normal Python comparison methods like __eq__() etc., but it would simplify their usage to auto-generate the obvious mappings when the @functools.total_ordering decorator is used. At the C level, this could probably be done without a performance penalty.

See https://docs.python.org/3/library/functools.html#functools.total_ordering

Since this only applies to cdef classes, it seems better to use a safe and dedicated separate decorator @cython.total_ordering than to rely on the generic Python decorator.

Approach:

  • Support the new decorator as a new directive in Options.py, similar to @no_gc_clear.
  • In generate_richcmp_function() (in ModuleNode.py), add default implementations of the missing comparison functions to the C switch statement if the directive is set.
  • Add a new test file tests/run/exttype_total_ordering.pyx, implement different cdef class configurations in it, and test their comparisons with Python doctests. You can also implement normal Python classes in these doctests for direct comparison of Python's own behaviour.
  • Run the tests with python[2|3] runtests.py -vv --debug exttype_total_ordering. The generated code can be found in the TEST_TMP/run/c/ directory.
@jdemeyer
Copy link
Contributor

jdemeyer commented Feb 5, 2018

than to rely on the generic Python decorator

...which is implemented very stupidly anyway, so +1 to improving it in Cython.

If you do this, then I would drop the automatic adding of __ne__ by default, but add it if @cython.total_ordering is given.

@scoder
Copy link
Contributor Author

scoder commented Feb 6, 2018 via email

@jdemeyer
Copy link
Contributor

jdemeyer commented Feb 6, 2018

Ah I see! The automatic __ne__ seems to be new in Python 3. Python 2 doesn't do that.

@scoder
Copy link
Contributor Author

scoder commented Feb 6, 2018 via email

@jdemeyer
Copy link
Contributor

jdemeyer commented Feb 6, 2018

Good to know, but I don't think it hurts anyone if we keep the Py3 behaviour, rather than implementing an explicit difference which pretty much no-one would notice, just for a legacy Python version.

Sure.

@holdenk
Copy link

holdenk commented Feb 28, 2018

Cool, I'd be happy to take this on as a starter issue if that would be cool with folks?

@scoder
Copy link
Contributor Author

scoder commented Feb 28, 2018

Sure! I updated the description with some pointers. Please ask back whenever something is unclear.

@TeamSpen210
Copy link
Contributor

Why is it necessary to have a Cython-specific version of the decorator? Aside from tracebacks the behaviour would be identical. The only reason I can think of would be to maintain monkeypatching capabilities for the module, which would be rather fragile anyway (since you need to ensure import orders)... I could see in future optimisations being implemented for other decorators, and the cython namespace getting excessively cluttered.

@scoder
Copy link
Contributor Author

scoder commented Feb 28, 2018 via email

@scoder
Copy link
Contributor Author

scoder commented May 26, 2021

Implemented in #3626. Documentation is missing. Should go here: https://cython.readthedocs.io/en/latest/src/userguide/special_methods.html#rich-comparison-operators

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants