Rich comparison functions of many builtin types include a block of boilerplate which can be consolidated in a macro. The macro can be useful for third-party extensions as well as CPython itself.

See this e-mail for a longer write-up: https://mail.python.org/pipermail/python-ideas/2015-March/032564.html

Here is a patch that uses the macro in all the places it can help.

Such macros would make the code cleaner. But I don't think it should be provided as a part of API. It isn't hard to implement, it doesn't provide essential functionality of Python, and it doesn't hide implementation defined CPython internals. I rather consider it as private helper, so it should be declared with underscore prefix and inside the #ifndef Py_LIMITED_API/#endif block.

I don't see an implementation of Py_RICHCOMPARE. In any case I think it would be better to make the op parameter the first parameter.

Hmm, at least for the version at

  https://mail.python.org/pipermail/python-ideas/2015-March/032564.html

I'm not sure if the optimizer will produce the same
code as for the switch statement.  Did you look at
the asm?

gcc -O3 generates a sequence of cmp-s.

Serhiy: Thanks for looking at this!
I think it should fall in the same category as Py_RETURN_TRUE or Py_RETURN_NONE. Sure, it's easy to reimplement, but a lot of extensions need it; why should everyone need to write the same code in a dozen different ways?
I specifically want this usable in third-party code.

The implementation of Py_RICHCOMPARE is in the first patch. The second is example use.
The signature mirrors richcmpfunc. Why would op be better as first argument?

Stefan: Which optimizer should I look at? Is it important to generate the same code? 

Sorry if I'm asking for something obvious, I'm not a regular here.

Attaching another patch:

- Leave _decimal alone per maintainer's wishes
- Fixes issues pointed out in the review
- Use Py_RICHCOMPARE also in _tkinter
- More improvements in the other affected modules

> I think it should fall in the same category as Py_RETURN_TRUE or
> Py_RETURN_NONE. Sure, it's easy to reimplement, but a lot of extensions
> need it; why should everyone need to write the same code in a dozen
> different ways? I specifically want this usable in third-party code.

The interface of Py_RETURN_TRUE is simple and unambiguous. Py_RICHCOMPARE is 
more complex and unobvious. One question is about the order of arguments 
(opcode as first argument looks better on my taste). Other question is about 
return value. Should it be an integer 0 or 1, Python object.

Stefan ask good question about implementation. A sequence of ifs can be less 
efficient than one switch.

Py_RETURN_TRUE and Py_RETURN_NONE are used hundreds times in CPython code and 
in any large extension (and can be used more). But the use case of 
Py_RICHCOMPARE is pretty limited.

> The implementation of Py_RICHCOMPARE is in the first patch.

Ah, I see the code in text patch, but on Rietveld it isn't visible. Perhaps 
Rietveld doesn't show some parts from the second patch.

Making it a function might help with the following issues:
- series of comparisons and PyBool_FromLong is less efficient than switch and Py_RETURN_*. But it would add a function call.
- it might be too complex for a macro

Do you think that would help?

As for the signature, I would like this to mirror richcmpfunc and PyObject_RichCompareBool. And returning PyObjexct*, not 0/1, is an important part of reducing boilerplate; in cases where 0/1 would be helpful you can easily work with cmp-style -1/0/1 values before using this to convert them.

Changed the macro to Py_RETURN_RICHCOMPARE. This is not an expression, allowing the use of a switch statement. On the other hand, it's even larger macro than before now.

From the discussion it seems that doing this correctly is tricky to do this correctly - another point for standardizing this.

I put everything in a single macro to ease review in Rietveld.

ping

Anything I can do to help move this forward?

From the discussion on the list:

- It needs to be a macro, not function, to support various types (unsigned long long, float; possibly C++ stuff with overriden operators)

- Another suggestion to change the order of arguments; I still think being the same as richcmp and PyObject_RichCompareBool is best.

I believe all the issues raised here and on the list are handled. Could anyone re-review the patch?
If the usage changes are too much, it's possible to only change Include/object.h and Doc/c-api/typeobj.rst, and leave the rest. Should I trim the patch that way?

Anything else I can do to help this get merged?

FWIW, I prefer the current form so that I don't have to constantly lookup to see exactly what the macro does.   

If this has been around from the beginning, it might have "eased" the writing by a minute or so.  But now, it will just be a barrier to maintenance.

Is it really not better to give the operation a name, rather than repeating the same ten lines every time? (Well, not the same -- all the modules code it a bit differently, but with the same meaning.)

I might be true that the types in Python itself are "done", but this is intended as part of the C API. There are still modules unported to Python 3, for which *now* is the beginning.

I'm -1 on this whole concept and I don't believe that it will make porting easier.  It takes longer to learn the macro, see what it does, write tests for it, etc than it takes to model ten lines of boilerplate code.

The macros make it harder for me and others to understand and maintain the code.  In this regard, Python has been getting worse (harder for new maintainers to look at code and know what it is doing).   Saving ten lines of clear code isn't a good motivation for going down this path.   C macros are infamous for a reason.

Well, as a newcomer, I think the macro makes it easier to both grok what the code does, and is about equally difficult when it comes to checking correctness of the code.
But I understand that's a subjective.

Marc-Andre, Barry, you expressed interest in the macro on the mailing list; do you still think it's a good idea?

@rhettinger: OTOH, a macro can provide uniformity and correctness.  If (as appears evident from the patch) those "10 lines of boilerplate" are actually implemented subtly differently each time, bugs can be easily introduced.  So a well written and documented macro can be both more readable and more correct.

On 14.05.2015 13:29, Petr Viktorin wrote:
> 
> Marc-Andre, Barry, you expressed interest in the macro on the mailing list; do you still think it's a good idea?

Yes.

The fact that the macro can save us more than a hundred lines
of code in Python itself is proof enough that it's useful to have.

Only once concept to learn, fewer bugs, one place to apply change
(should they become necessary), etc. etc.

This is a standard case of refactoring to simplify code and also
a standard case where we use macros in Python.

What can I, not a core developer, do to resolve this disagreement?

Should I submit a PEP?

You don't need a PEP.  If Barry and Marc-Andre want this to go forward, I won't hold it back.

The problem with this macro is that most of the time it takes the
standard cmp return value {-1,0,1} and converts that into a bool.

For this use case, it might be more appropriate to use a
static inline function Py_cmp_to_bool().

To put it differently, the macro mostly does not perform the
actual rich comparison but just post-processes the result.


I don't like the dual use of converting cmp() return values
and performing actual comparisons on integers, so -1 on
the concept.

On 18.05.2015 15:46, Stefan Krah wrote:
> 
> Stefan Krah added the comment:
> 
> The problem with this macro is that most of the time it takes the
> standard cmp return value {-1,0,1} and converts that into a bool.
> 
> For this use case, it might be more appropriate to use a
> static inline function Py_cmp_to_bool().
> 
> To put it differently, the macro mostly does not perform the
> actual rich comparison but just post-processes the result.
> 
> 
> I don't like the dual use of converting cmp() return values
> and performing actual comparisons on integers, so -1 on
> the concept.

I don't follow you. The macro performs a similar task as
that of e.g. Py_RETURN_TRUE/Py_RETURN_FALSE/etc. that is
to reduce boilerplate code and in this particular case
also to remove potential sources of bugs in both the Python
interpreter itself and C extensions written for it.

I mean it's clearer to have:

    result = long_compare(self, other);
    return Py_cmp_to_bool(result, op);

than:

    result = long_compare(self, other);
    Py_RETURN_RICHCOMPARE(result, 0, op);


This is because in other places, like the proposed use
case in

  https://mail.python.org/pipermail/python-ideas/2015-March/032564.html ,


the macro actually *performs* the "rich" comparison. In the above case
it just *converts* the result of long_compare().


Maybe the distinction does not matter in practice, but I'm not
too happy with it.

Conceptually there's a distinction between the two cases, but you can implement one in terms of the other, so I don't think it's worth adding two functions/macros here. So let's pick the better API.

"Py_cmp_to_bool" is better if you already have a cmp-style result. Python code is full of cmp-style results, but I think a big reason is that py2 required them, and (rightly) nobody wants to rewrite the algorithms. I believe the py3 way of passing in the operator is better API.

I've seen (a - b) far too many times, which gives the right result in most but *not all* cases. (Think small floats where the difference is coerced to int for the Py_cmp_to_bool function. Or int overflow.)
The correct ways to get a cmp-style result are "(a > b) - (a < b)" or "(a < b) ? -1 : (a > b)". Do we want to add a function that requires you to write, read, and understand that?

It seems that it won't be easy to find an API that pleases everyone.

I don't want to prolong the discussion much, but if the macro goes in,
returning PyErr_BadArgument() in the default case would be better than
NotImplemented.

assert(0) would be fine as well.

Well, in my opinion NotImplemented is a good value for "unknown operation", but I'll be happy to change to PyErr_BadArgument(); return NULL; if there's support for that.

NotImplemented is a non-error return value that's used when the
objects cannot be compared, e.g. when the function receives Py_LT
but the objects are unorderable.

Getting a value outside {Py_EQ, ...} is a hard error that cannot
occur in a correct program.

Here is a version with PyErr_BadArgument.

Just a reminder: if you want this to be in Python 3.5, please review the patch

Sent a PR against the master branch. What do you think about it?

Would it make sense as well for python 3.6 now?

Assigning to myself to review.

To add some context that hasn't come up previously, the essential idea for this macro originated in the "Py3C" extension module compatibility project, where it helps authors of Python 2 extension modules update their projects to be compatible with Python 3: https://py3c.readthedocs.io/en/latest/reference.html#comparison-helpers

As with most such pattern extractions, the key intent is to make it easier for developers to correctly implement a Python-specific protocol in C by replacing the current copy-and-paste handling of such cases with the use of C's preprocessor.

PR has been rebased on top of master and also blurbified.

IMO, "op" should be the first argument to the macro, since that reflects the tp_richcmp API.

Both tp_richcompare and PyObject_RichCompareBool have op as the last argument:

https://docs.python.org/3/c-api/object.html#c.PyObject_RichCompareBool
https://docs.python.org/3/c-api/typeobj.html?highlight=tp_richcompare#c.PyTypeObject.tp_richcompare

On Tue, Oct 24, 2017, at 02:23, Petr Viktorin wrote:
> 
> Petr Viktorin <encukou@gmail.com> added the comment:
> 
> Both tp_richcompare and PyObject_RichCompareBool have op as the last
> argument:

Yes, indeed. Sorry, I wasn't thinking straight.

New changeset e8b19656396381407ad91473af5da8b0d4346e88 by Nick Coghlan (stratakis) in branch 'master':
bpo-23699: Use a macro to reduce boilerplate code in rich comparison functions (GH-793)
https://github.com/python/cpython/commit/e8b19656396381407ad91473af5da8b0d4346e88