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

Add a macro to ease writing rich comparisons #67887

Closed
encukou opened this issue Mar 18, 2015 · 36 comments
Closed

Add a macro to ease writing rich comparisons #67887

encukou opened this issue Mar 18, 2015 · 36 comments
Assignees
Labels
3.7 interpreter-core type-feature

Comments

@encukou
Copy link
Member

@encukou encukou commented Mar 18, 2015

BPO 23699
Nosy @malemburg, @warsaw, @rhettinger, @ncoghlan, @benjaminp, @encukou, @skrah, @serhiy-storchaka, @stratakis
PRs
  • #793
  • Files
  • 0001-Define-Py_RICHCOMPARE-to-ease-writing-rich-compariso.patch: Patch adding the Py_RICHCOMPARE macro
  • 0002-Use-Py_RICHCOMPARE-in-rich-comparisons.patch
  • 0002-Use-Py_RICHCOMPARE-in-rich-comparisons.patch: Patch adding the Py_RICHCOMPARE macro, v. 2
  • Use-a-macro-to-reduce-boilerplate-in-rich-comparison.patch
  • richcompare-macro-badargument.patch
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/ncoghlan'
    closed_at = <Date 2017-11-02.10:33:57.218>
    created_at = <Date 2015-03-18.13:43:22.222>
    labels = ['interpreter-core', 'type-feature', '3.7']
    title = 'Add a macro to ease writing rich comparisons'
    updated_at = <Date 2017-11-02.10:33:57.218>
    user = 'https://github.com/encukou'

    bugs.python.org fields:

    activity = <Date 2017-11-02.10:33:57.218>
    actor = 'ncoghlan'
    assignee = 'ncoghlan'
    closed = True
    closed_date = <Date 2017-11-02.10:33:57.218>
    closer = 'ncoghlan'
    components = ['Interpreter Core']
    creation = <Date 2015-03-18.13:43:22.222>
    creator = 'petr.viktorin'
    dependencies = []
    files = ['38541', '38542', '38605', '38657', '39455']
    hgrepos = []
    issue_num = 23699
    keywords = ['patch']
    message_count = 36.0
    messages = ['238441', '238443', '238454', '238457', '238461', '238658', '238673', '238674', '238681', '239042', '241861', '243095', '243108', '243116', '243120', '243176', '243179', '243183', '243264', '243271', '243479', '243480', '243481', '243485', '243665', '243707', '243748', '243764', '243836', '290055', '294734', '304821', '304863', '304884', '304962', '305412']
    nosy_count = 9.0
    nosy_names = ['lemburg', 'barry', 'rhettinger', 'ncoghlan', 'benjamin.peterson', 'petr.viktorin', 'skrah', 'serhiy.storchaka', 'cstratak']
    pr_nums = ['793']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue23699'
    versions = ['Python 3.7']

    @encukou
    Copy link
    Member Author

    @encukou encukou commented Mar 18, 2015

    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

    @encukou encukou added the type-feature label Mar 18, 2015
    @encukou
    Copy link
    Member Author

    @encukou encukou commented Mar 18, 2015

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

    @serhiy-storchaka
    Copy link
    Member

    @serhiy-storchaka serhiy-storchaka commented Mar 18, 2015

    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.

    @serhiy-storchaka serhiy-storchaka added the interpreter-core label Mar 18, 2015
    @skrah
    Copy link
    Mannequin

    @skrah skrah mannequin commented Mar 18, 2015

    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?

    @serhiy-storchaka
    Copy link
    Member

    @serhiy-storchaka serhiy-storchaka commented Mar 18, 2015

    gcc -O3 generates a sequence of cmp-s.

    @encukou
    Copy link
    Member Author

    @encukou encukou commented Mar 20, 2015

    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.

    @encukou
    Copy link
    Member Author

    @encukou encukou commented Mar 20, 2015

    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

    @serhiy-storchaka
    Copy link
    Member

    @serhiy-storchaka serhiy-storchaka commented Mar 20, 2015

    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.

    @encukou
    Copy link
    Member Author

    @encukou encukou commented Mar 20, 2015

    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.

    @encukou
    Copy link
    Member Author

    @encukou encukou commented Mar 23, 2015

    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.

    @encukou
    Copy link
    Member Author

    @encukou encukou commented Apr 23, 2015

    ping

    Anything I can do to help move this forward?

    @encukou
    Copy link
    Member Author

    @encukou encukou commented May 13, 2015

    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?

    @rhettinger
    Copy link
    Contributor

    @rhettinger rhettinger commented May 13, 2015

    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.

    @encukou
    Copy link
    Member Author

    @encukou encukou commented May 13, 2015

    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.

    @rhettinger
    Copy link
    Contributor

    @rhettinger rhettinger commented May 13, 2015

    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.

    @encukou
    Copy link
    Member Author

    @encukou encukou commented May 14, 2015

    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?

    @warsaw
    Copy link
    Member

    @warsaw warsaw commented May 14, 2015

    @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.

    @malemburg
    Copy link
    Member

    @malemburg malemburg commented May 14, 2015

    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.

    @encukou
    Copy link
    Member Author

    @encukou encukou commented May 15, 2015

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

    Should I submit a PEP?

    @rhettinger
    Copy link
    Contributor

    @rhettinger rhettinger commented May 15, 2015

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

    @skrah
    Copy link
    Mannequin

    @skrah skrah mannequin commented May 18, 2015

    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.

    @malemburg
    Copy link
    Member

    @malemburg malemburg commented May 18, 2015

    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.

    @skrah
    Copy link
    Mannequin

    @skrah skrah mannequin commented May 18, 2015

    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.

    @encukou
    Copy link
    Member Author

    @encukou encukou commented May 18, 2015

    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?

    @skrah
    Copy link
    Mannequin

    @skrah skrah mannequin commented May 20, 2015

    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.

    @encukou
    Copy link
    Member Author

    @encukou encukou commented May 20, 2015

    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.

    @skrah
    Copy link
    Mannequin

    @skrah skrah mannequin commented May 21, 2015

    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.

    @encukou
    Copy link
    Member Author

    @encukou encukou commented May 21, 2015

    Here is a version with PyErr_BadArgument.

    @encukou
    Copy link
    Member Author

    @encukou encukou commented May 22, 2015

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

    @stratakis
    Copy link
    Mannequin

    @stratakis stratakis mannequin commented Mar 23, 2017

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

    Would it make sense as well for python 3.6 now?

    @ncoghlan
    Copy link
    Contributor

    @ncoghlan ncoghlan commented May 30, 2017

    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.

    @ncoghlan ncoghlan added the 3.7 label May 30, 2017
    @ncoghlan ncoghlan self-assigned this May 30, 2017
    @stratakis
    Copy link
    Mannequin

    @stratakis stratakis mannequin commented Oct 23, 2017

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

    @benjaminp
    Copy link
    Contributor

    @benjaminp benjaminp commented Oct 24, 2017

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

    @encukou
    Copy link
    Member Author

    @encukou encukou commented Oct 24, 2017

    @benjaminp
    Copy link
    Contributor

    @benjaminp benjaminp commented Oct 25, 2017

    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.

    @ncoghlan
    Copy link
    Contributor

    @ncoghlan ncoghlan commented Nov 2, 2017

    New changeset e8b1965 by Nick Coghlan (stratakis) in branch 'master':
    bpo-23699: Use a macro to reduce boilerplate code in rich comparison functions (GH-793)
    e8b1965

    @ncoghlan ncoghlan closed this Nov 2, 2017
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.7 interpreter-core type-feature
    Projects
    None yet
    Development

    No branches or pull requests

    7 participants