classification
Title: Implement comparison operators for range objects
Type: enhancement Stage: resolved
Components: Interpreter Core Versions: Python 3.3
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: mark.dickinson Nosy List: ezio.melotti, haypo, mark.dickinson, python-dev, smarnach
Priority: normal Keywords: needs review, patch

Created on 2011-10-17 14:52 by smarnach, last changed 2011-10-23 18:54 by mark.dickinson. This issue is now closed.

Files
File name Uploaded Description Edit
range-compare.patch smarnach, 2011-10-17 14:52 review
range-compare-v2.patch smarnach, 2011-10-17 16:55 review
range-compare-v3.patch smarnach, 2011-10-18 11:31 review
range-compare-v4.patch mark.dickinson, 2011-10-22 15:38 review
Messages (14)
msg145705 - (view) Author: Sven Marnach (smarnach) Date: 2011-10-17 14:52
It seems some sort of consensus on how to compare range objects has emerged from the python-ideas discussion on comparison of range objects [1].

The attached patch defines '==' and '!=' for range object equality based on the sequence of values they represent.  The other comparison operators are left NotImplemented.

[1]: http://mail.python.org/pipermail/python-ideas/2011-October/012376.html
msg145714 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2011-10-17 15:28
Nice patch!  I put some comments on Rietveld.
msg145727 - (view) Author: Sven Marnach (smarnach) Date: 2011-10-17 16:55
Mark, thanks for your comments.  Here's a new version of the patch, I answer on Rietveld.
msg145746 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2011-10-17 18:23
The new patch looks fine;  I'd still like to have the more explicit reference counting in range_hash (see replies on Rietveld).

A few more things:

- The patch needs a Misc/NEWS entry before committing;  it probably deserves a line in Doc/whatsnew/3.3.rst, too.

- The doc update should have a ".. versionchanged:: 3.3" note.

- Maybe the range_compare function could be renamed to something that makes it clearer it's just doing an equality comparison rather than a generalized comparison.  'range_equality_check'?  Or just 'range_equal' or 'range_equality'?
msg145796 - (view) Author: Sven Marnach (smarnach) Date: 2011-10-18 11:31
Mark, thanks again for your comments.  (I never looked at the Python source code before, so tey are highly appreciated.)  I uploaded a new version of the patch hopefully.
msg146015 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2011-10-20 13:19
I get a test failure in test_hash (which is checking exactly that the hash(range) uses the default object hash, so that test is clearly out of date now).  Apart from that, the latest patch looks good to me.

I'm going to give this a couple of days in case anyone else has any comments, then I'll aim to commit it (with the extra test_hash fix) this weekend.

Thanks for all the work on this!
msg146016 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2011-10-20 13:23
+    one = PyLong_FromLong(1);
+    if (!one)
+        return -1;
+    cmp_result = PyObject_RichCompareBool(r0->length, one, Py_EQ);
+    Py_DECREF(one);

If would be nice to have a PyLong_CompareLong() function.
msg146172 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2011-10-22 15:38
I've taken the liberty of updating the patch, with a few minor changes:

range_equality -> range_equals (like range_contains)
move identity check into range_equals
move comments before the code they describe (PEP7)
add whatsnew entry
remove check that range.__hash__ matches object.__hash__ in test_hash
change assertEqual into assertIs where appropriate (as suggested by Ezio)
additional comments and tests in Lib/test/test_range (ditto)

Sven, Ezio:  okay to apply this?
msg146173 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2011-10-22 15:40
Hmm.  Why does my patch not get a 'review' button?
msg146174 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2011-10-22 15:40
Ah, there it is. Never mind. :-)
msg146198 - (view) Author: Sven Marnach (smarnach) Date: 2011-10-22 22:51
Thanks for the updates, Mark.  I was just about to look into this again.  The changes are fine with me.
msg146199 - (view) Author: Sven Marnach (smarnach) Date: 2011-10-22 22:54
Victor Stinner wrote:
> If would be nice to have a PyLong_CompareLong() function.

In most cases, global variables Py_Zero and Py_One would be enough to simplify this kind of code.
msg146244 - (view) Author: Roundup Robot (python-dev) Date: 2011-10-23 18:53
New changeset 479a7dd1ea6a by Mark Dickinson in branch 'default':
Issue #13201: equality for range objects is now based on equality of the underlying sequences.  Thanks Sven Marnach for the patch.
http://hg.python.org/cpython/rev/479a7dd1ea6a
msg146245 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2011-10-23 18:53
> In most cases, global variables Py_Zero and Py_One would be enough to
> simplify this kind of code.

Agreed.
History
Date User Action Args
2011-11-18 09:42:18floxlinkissue13423 superseder
2011-10-23 18:54:23mark.dickinsonsetstatus: open -> closed
resolution: fixed
stage: commit review -> resolved
2011-10-23 18:53:55mark.dickinsonsetmessages: + msg146245
2011-10-23 18:53:12python-devsetnosy: + python-dev
messages: + msg146244
2011-10-22 22:54:41smarnachsetmessages: + msg146199
2011-10-22 22:51:36smarnachsetmessages: + msg146198
2011-10-22 15:40:45mark.dickinsonsetmessages: + msg146174
2011-10-22 15:40:19mark.dickinsonsetmessages: + msg146173
2011-10-22 15:38:36mark.dickinsonsetfiles: + range-compare-v4.patch

messages: + msg146172
2011-10-20 13:23:09hayposetnosy: + haypo
messages: + msg146016
2011-10-20 13:19:50mark.dickinsonsetmessages: + msg146015
stage: patch review -> commit review
2011-10-18 11:31:47smarnachsetfiles: + range-compare-v3.patch

messages: + msg145796
2011-10-17 18:23:41mark.dickinsonsetmessages: + msg145746
2011-10-17 16:55:28smarnachsetfiles: + range-compare-v2.patch

messages: + msg145727
2011-10-17 15:34:15mark.dickinsonsetassignee: mark.dickinson
2011-10-17 15:31:46ezio.melottisetkeywords: + patch, needs review
nosy: + ezio.melotti
2011-10-17 15:28:45mark.dickinsonsetmessages: + msg145714
2011-10-17 14:54:40mark.dickinsonsetversions: + Python 3.3
nosy: + mark.dickinson

components: + Interpreter Core
type: enhancement
stage: patch review
2011-10-17 14:52:53smarnachcreate