classification
Title: Add a macro to ease writing rich comparisons
Type: enhancement Stage: resolved
Components: Interpreter Core Versions: Python 3.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: ncoghlan Nosy List: barry, benjamin.peterson, cstratak, lemburg, ncoghlan, petr.viktorin, rhettinger, serhiy.storchaka, skrah
Priority: normal Keywords: patch

Created on 2015-03-18 13:43 by petr.viktorin, last changed 2017-11-02 10:33 by ncoghlan. This issue is now closed.

Files
File name Uploaded Description Edit
0001-Define-Py_RICHCOMPARE-to-ease-writing-rich-compariso.patch petr.viktorin, 2015-03-18 13:43 Patch adding the Py_RICHCOMPARE macro review
0002-Use-Py_RICHCOMPARE-in-rich-comparisons.patch petr.viktorin, 2015-03-18 13:45 review
0002-Use-Py_RICHCOMPARE-in-rich-comparisons.patch petr.viktorin, 2015-03-20 13:54 Patch adding the Py_RICHCOMPARE macro, v. 2 review
Use-a-macro-to-reduce-boilerplate-in-rich-comparison.patch petr.viktorin, 2015-03-23 15:32 review
richcompare-macro-badargument.patch petr.viktorin, 2015-05-21 16:49 review
Pull Requests
URL Status Linked Edit
PR 793 merged cstratak, 2017-03-23 17:36
Messages (36)
msg238441 - (view) Author: Petr Viktorin (petr.viktorin) * (Python committer) Date: 2015-03-18 13:43
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
msg238443 - (view) Author: Petr Viktorin (petr.viktorin) * (Python committer) Date: 2015-03-18 13:45
Here is a patch that uses the macro in all the places it can help.
msg238454 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-03-18 15:34
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.
msg238457 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2015-03-18 16:42
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?
msg238461 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-03-18 17:09
gcc -O3 generates a sequence of cmp-s.
msg238658 - (view) Author: Petr Viktorin (petr.viktorin) * (Python committer) Date: 2015-03-20 12:43
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.
msg238673 - (view) Author: Petr Viktorin (petr.viktorin) * (Python committer) Date: 2015-03-20 13:54
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
msg238674 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-03-20 13:56
> 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.
msg238681 - (view) Author: Petr Viktorin (petr.viktorin) * (Python committer) Date: 2015-03-20 14:33
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.
msg239042 - (view) Author: Petr Viktorin (petr.viktorin) * (Python committer) Date: 2015-03-23 15:32
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.
msg241861 - (view) Author: Petr Viktorin (petr.viktorin) * (Python committer) Date: 2015-04-23 10:34
ping

Anything I can do to help move this forward?
msg243095 - (view) Author: Petr Viktorin (petr.viktorin) * (Python committer) Date: 2015-05-13 14:48
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?
msg243108 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2015-05-13 17:52
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.
msg243116 - (view) Author: Petr Viktorin (petr.viktorin) * (Python committer) Date: 2015-05-13 19:02
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.
msg243120 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2015-05-13 21:23
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.
msg243176 - (view) Author: Petr Viktorin (petr.viktorin) * (Python committer) Date: 2015-05-14 11:29
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?
msg243179 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2015-05-14 11:52
@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.
msg243183 - (view) Author: Marc-Andre Lemburg (lemburg) * (Python committer) Date: 2015-05-14 12:05
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.
msg243264 - (view) Author: Petr Viktorin (petr.viktorin) * (Python committer) Date: 2015-05-15 12:41
What can I, not a core developer, do to resolve this disagreement?

Should I submit a PEP?
msg243271 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2015-05-15 15:02
You don't need a PEP.  If Barry and Marc-Andre want this to go forward, I won't hold it back.
msg243479 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2015-05-18 13:46
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.
msg243480 - (view) Author: Marc-Andre Lemburg (lemburg) * (Python committer) Date: 2015-05-18 14:04
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.
msg243481 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2015-05-18 14:23
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.
msg243485 - (view) Author: Petr Viktorin (petr.viktorin) * (Python committer) Date: 2015-05-18 14:55
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?
msg243665 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2015-05-20 13:55
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.
msg243707 - (view) Author: Petr Viktorin (petr.viktorin) * (Python committer) Date: 2015-05-20 23:55
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.
msg243748 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2015-05-21 13:21
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.
msg243764 - (view) Author: Petr Viktorin (petr.viktorin) * (Python committer) Date: 2015-05-21 16:49
Here is a version with PyErr_BadArgument.
msg243836 - (view) Author: Petr Viktorin (petr.viktorin) * (Python committer) Date: 2015-05-22 16:13
Just a reminder: if you want this to be in Python 3.5, please review the patch
msg290055 - (view) Author: Charalampos Stratakis (cstratak) * Date: 2017-03-23 17:37
Sent a PR against the master branch. What do you think about it?

Would it make sense as well for python 3.6 now?
msg294734 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2017-05-30 03:02
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.
msg304821 - (view) Author: Charalampos Stratakis (cstratak) * Date: 2017-10-23 16:50
PR has been rebased on top of master and also blurbified.
msg304863 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2017-10-24 06:38
IMO, "op" should be the first argument to the macro, since that reflects the tp_richcmp API.
msg304884 - (view) Author: Petr Viktorin (petr.viktorin) * (Python committer) Date: 2017-10-24 09:23
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
msg304962 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2017-10-25 06:00
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.
msg305412 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2017-11-02 10:33
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
History
Date User Action Args
2017-11-02 10:33:57ncoghlansetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2017-11-02 10:33:00ncoghlansetmessages: + msg305412
2017-10-25 06:00:47benjamin.petersonsetmessages: + msg304962
2017-10-24 09:23:23petr.viktorinsetmessages: + msg304884
2017-10-24 06:38:57benjamin.petersonsetnosy: + benjamin.peterson
messages: + msg304863
2017-10-23 16:50:49cstrataksetmessages: + msg304821
2017-05-30 03:02:41ncoghlansetversions: + Python 3.7, - Python 3.5
nosy: + ncoghlan

messages: + msg294734

assignee: ncoghlan
stage: patch review
2017-03-23 17:37:14cstrataksetnosy: + cstratak
messages: + msg290055
2017-03-23 17:36:10cstrataksetpull_requests: + pull_request698
2015-07-21 07:11:01ethan.furmansetnosy: - ethan.furman
2015-05-22 16:13:37petr.viktorinsetmessages: + msg243836
2015-05-21 16:49:42petr.viktorinsetfiles: + richcompare-macro-badargument.patch

messages: + msg243764
2015-05-21 13:21:09skrahsetmessages: + msg243748
2015-05-20 23:55:41petr.viktorinsetmessages: + msg243707
2015-05-20 13:55:12skrahsetmessages: + msg243665
2015-05-18 14:55:56petr.viktorinsetmessages: + msg243485
2015-05-18 14:23:57skrahsetmessages: + msg243481
2015-05-18 14:04:27lemburgsetmessages: + msg243480
2015-05-18 13:46:09skrahsetmessages: + msg243479
2015-05-15 15:02:20rhettingersetmessages: + msg243271
2015-05-15 12:41:48petr.viktorinsetmessages: + msg243264
2015-05-14 12:05:06lemburgsetmessages: + msg243183
2015-05-14 11:52:12barrysetmessages: + msg243179
2015-05-14 11:29:34petr.viktorinsetnosy: + lemburg
messages: + msg243176
2015-05-13 21:23:55rhettingersetmessages: + msg243120
2015-05-13 19:02:22petr.viktorinsetmessages: + msg243116
2015-05-13 17:52:12rhettingersetnosy: + rhettinger
messages: + msg243108
2015-05-13 14:48:57petr.viktorinsetmessages: + msg243095
2015-04-28 14:53:30barrysetnosy: + barry
2015-04-23 10:34:41petr.viktorinsetmessages: + msg241861
2015-03-23 15:32:53petr.viktorinsetfiles: + Use-a-macro-to-reduce-boilerplate-in-rich-comparison.patch

messages: + msg239042
2015-03-20 14:33:30petr.viktorinsetmessages: + msg238681
2015-03-20 13:56:02serhiy.storchakasetmessages: + msg238674
2015-03-20 13:54:25petr.viktorinsetfiles: + 0002-Use-Py_RICHCOMPARE-in-rich-comparisons.patch

messages: + msg238673
2015-03-20 12:43:30petr.viktorinsetmessages: + msg238658
2015-03-18 17:09:31serhiy.storchakasetmessages: + msg238461
2015-03-18 16:42:06skrahsetmessages: + msg238457
2015-03-18 16:33:47skrahsetnosy: + skrah
2015-03-18 16:24:35ethan.furmansetnosy: + ethan.furman
2015-03-18 15:34:33serhiy.storchakasetversions: + Python 3.5
nosy: + serhiy.storchaka

messages: + msg238454

components: + Interpreter Core
2015-03-18 13:45:11petr.viktorinsetfiles: + 0002-Use-Py_RICHCOMPARE-in-rich-comparisons.patch

messages: + msg238443
2015-03-18 13:43:22petr.viktorincreate