classification
Title: Bug in range() function for large values
Type: behavior Stage: resolved
Components: Interpreter Core, Library (Lib) Versions: Python 2.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: mark.dickinson Nosy List: akitada, belopolsky, christian.heimes, josm, loewis, mark.dickinson, rhettinger, robertwb, zanella
Priority: normal Keywords: 26backport, patch

Created on 2007-12-01 00:28 by robertwb, last changed 2010-05-07 13:26 by mark.dickinson. This issue is now closed.

Files
File name Uploaded Description Edit
bad_range.py robertwb, 2007-12-01 00:28
bltinmodule2.diff robertwb, 2010-05-01 19:02 modification of bltinmodule.diff to reject floats
issue1533.diff belopolsky, 2010-05-03 03:52
issue1533_metd.diff mark.dickinson, 2010-05-04 12:33
issue1533-py3k-tests.diff belopolsky, 2010-05-04 18:30 Additional tests for py3k
issue1533-release26-maint.diff belopolsky, 2010-05-06 17:42 2.6 backport
Messages (57)
msg58034 - (view) Author: Robert Bradshaw (robertwb) Date: 2007-12-01 00:28
Range accepts arguments coerce-able into ints via __int__, but rejects
arguments coerce-able into longs but to large to fit into an int. 

The problem is in handle_range_longs in bltinmodule.c:1527-1541. If they
type is not an int or long, it should try to make it so before failing
(for consistency with smaller values at least). 

Attached is a file that reproduces this bug.
msg58039 - (view) Author: John Smith (josm) Date: 2007-12-01 01:37
Is this a bug?
  print range(MyInt(2**64), MyInt(2**64+10)) 
produced
 TypeError: range() integer start argument expected, got instance.

print range(int(MyInt(2**64)), int(MyInt(2**64+10)))
should work.
msg58040 - (view) Author: Robert Bradshaw (robertwb) Date: 2007-12-01 01:42
Yes, that is a workaround, but 

range(MyInt(n), MyInt(n+10))

should work for any valid value of n, not just some of them.
msg62617 - (view) Author: Rafael Zanella (zanella) Date: 2008-02-21 11:16
FWIW, using xrange() it seems to give the proper error message:

Traceback (most recent call last):
  File "bad_range.py", line 12, in <module>
    print xrange(MyInt(2**64), MyInt(2**64+10))
OverflowError: long int too large to convert to int
msg62631 - (view) Author: Robert Bradshaw (robertwb) Date: 2008-02-21 17:02
Yes, the error for xrange is more illustrative of the problem, but just
shows that xrange has this a too. Why should xrange be invalid for
non-word sized values (especially as range works)? Incidentally, just a
week ago I had to write my own iterator for a project because xrange
couldn't handle large values.
msg62898 - (view) Author: Rafael Zanella (zanella) Date: 2008-02-24 13:19
According to the documentation
(http://docs.python.org/dev/library/functions.html) "The arguments must
be plain integers", so I think the wrong thing here is to run the
object's __int__() under the range()'s hood. I think the right thing to
do would be to explicitly invoke int() on passing an non-int argument as
parameter.
msg62937 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2008-02-24 21:03
This is related to issue1540617 and issue1546078.

issue1540617 contains a simple patch that extend acceptable range of 
argument to +/-2**63

issue1546078 contains a complete long integer support implementation and  
was accepted in Py3k.

It looks like all three issues can be closed by either accepting or 
rejecting issue1540617 patch for 2.6 and marking issue1546078 patch as 
accepted for Py3k.
msg62938 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2008-02-24 21:07
I'm -10 on the patch in issue1540617 ( +/-2**63).

Reason: It's a good thing that the range of "range" is limited since it
returns a list of integers. range(2**32) allocates (2**32)*16 bytes +
small overhead for ints plus the space for the list (probably
(2**32)*sizeof(ptr) which is 4 or 8 bytes). So far the memory for the
ints is *never* returned to the system. I'm working on the problem.
msg62943 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2008-02-24 21:44
Christian,

I was probably a bit sloppy using "range" instead of "xrange," but issue1540617 is limited to xrange only.  Are you still -10 on extending 
xrange on 64-bit platforms to +/- 2**63?  If so, what is your position 
on backporting py3k's unlimited range implementation?
msg62945 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2008-02-24 22:00
> So far the memory for the ints is *never* returned
> to the system. I'm working on the problem.

Christian,

Are you working on the memory problem or on this issue? I think I have a 
solution to OP's problem, but don't want to duplicate your effort.
msg62947 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2008-02-24 22:14
I'm working on the memory problem. See #2039 and #2013.

xrange is a totally different story. I'm +0 on changing xrange. Is
Python 3.0's range function compatible with xrange? If the answer is
yes, we may reuse the code for an unlimited xrange.
msg62950 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2008-02-24 23:08
Attached patch addresses OP's issue:

$ ./python.exe bad_range.py
[8, 9, 10, 11, 12, 13, 14, 15, 16, 17]
here
[18446744073709551616L, 18446744073709551617L, 18446744073709551618L, 
18446744073709551619L, 18446744073709551620L, 18446744073709551621L, 
18446744073709551622L, 18446744073709551623L, 18446744073709551624L, 
18446744073709551625L]
[18446744073709551616L, 18446744073709551617L, 18446744073709551618L, 
18446744073709551619L, 18446744073709551620L, 18446744073709551621L, 
18446744073709551622L, 18446744073709551623L, 18446744073709551624L, 
18446744073709551625L]

The only existing test that fails is range(1e100, 1e101, 1e101) 
producing a TypeError. It will now produce

>>> range(1e100, 1e101, 1e101)
__main__:1: DeprecationWarning: integer argument expected, got float
[10000000000000000159028911097599180468360808563945281389781327557747838
772170381060813469985856815104L]

Note that range(1e100, 1e101) would still fail:
>>> range(1e100, 1e101)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
OverflowError: range() result has too many items

An alternative solution would be to disallow non-ints regardless of 
their value, but that is more likely to break someone's code.
msg62989 - (view) Author: Robert Bradshaw (robertwb) Date: 2008-02-25 17:52
Alexander Belopolsky's patch looks like the right fix for range() to me. 

The xrange limits still hold, but that should probably be a separate
issue/ticket.
msg76623 - (view) Author: Akira Kitada (akitada) Date: 2008-11-29 20:28
I'm just curious to know which is the right behavior.

# Python 3.0
Traceback (most recent call last):
  File "bad_range.py", line 7, in <module>
    print(range(MyInt(2**3), MyInt(2**3+10)))
TypeError: 'MyInt' object cannot be interpreted as an integer

# Python 2.7a0
[8, 9, 10, 11, 12, 13, 14, 15, 16, 17]
here
[18446744073709551616L, 18446744073709551617L, 18446744073709551618L,
18446744073709551619L, 18446744073709551620L, 18446744073709551621L,
18446744073709551622L, 18446744073709551623L, 18446744073709551624L,
18446744073709551625L]
Traceback (most recent call last):
  File "bad_range.py", line 11, in <module>
    print(range(MyInt(2**64), MyInt(2**64+10)))
TypeError: range() integer start argument expected, got instance.
msg76624 - (view) Author: Robert Bradshaw (robertwb) Date: 2008-11-29 20:45
I think *both* behaviors are wrong, the 3.0 one is backwards
incompatible, and the 2.7 one is inconsistent (accepting MyInt if it's <
32 bits, rejecting it for > 64 bits). 

For our particular use case, it is very annoying to not be able to use
non-ints. It goes against the principle duck typing by simply defining
the __int__ and __index__ methods.
msg76630 - (view) Author: Akira Kitada (akitada) Date: 2008-11-29 22:53
Updating versions.
msg77509 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2008-12-10 08:52
This change is out of scope for 2.5.3 (plus, the desired behavior still
seems to be debated)
msg104680 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2010-05-01 00:34
As far as I can tell there's no bug in 3.x:  the 3.x range happily accepts an instance of a  class that defines __index__.
msg104681 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2010-05-01 00:53
Currently, in trunk, I get:

>>> range(0.0, 11.0, 1.1)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: range() integer start argument expected, got float.

But with Alexander's patch on trunk, I get:

>>> range(0.0, 11.0, 1.1)
[0L, 1L, 2L, 3L, 4L, 5L, 6L, 7L, 8L, 9L, 10L]

I'm not sure whether this is intentional or not, but IIRC it was a very deliberate choice not to allow float arguments to range (especially when, as here, the arguments are simply being truncated).  I don't think this is an acceptable change for 2.7 (still less for 2.6).

Any patch for this issue should not change the behaviour for small arguments.

IMO, the *right* solution is to convert arguments via __index__ when possible (as 3.x appears to already do).  However, this would be a new feature.  I suggest closing this as a 'won't fix'.
msg104682 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2010-05-01 01:05
> IIRC, it was a very deliberate choice not to allow float arguments to range

Ignore this bit.  IDRC.  It was a deliberate choice not to let something range(0.0, 1.0, 0.1) work to give [0.0, 0.1, ...], since the usual floating-point difficulties give uncertainty about the endpoint.

That float arguments were allowed (and silently truncated to integers) is merely unfortunate.  :)  And it's no longer permitted in 2.7;  I wouldn't want to go back to permitting float arguments here.

I'll set this to pending;  it should be closed unless someone comes up with a simple fix in the near future.
msg104687 - (view) Author: Alexander Belopolsky (Alexander.Belopolsky) Date: 2010-05-01 02:01
I agree that this issue should be closed with no further action, but for historical accuracy the resolution should be "out of date" rather than "won't fix".  The original bug was about range() behavior when it get arguments that are not ints, but "coerce-able into ints via __int__".  Since range() no longer accepts such arguments, the issue is moot and there is nothing to fix or not fix here.

As a pie in the sky idea, I always wanted a range function that would work on any arguments that support addition and ordering.  For example range(date(2010,1,1), date(2010, 2, 1), timedelta(7)) to return all Fridays in January, 2010.  However, since advent of generator functions, this became simply

def range(start, stop, step):
   while start < stop:
      yield start
      start += step

and thus unnecessary for stdlib.
msg104695 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2010-05-01 07:50
Alexander: range *does* still accept such arguments (in 2.7);  just not floats:

>>> from decimal import Decimal
>>> range(Decimal(20), Decimal(20))
[]
>>> range(Decimal('1e100'), Decimal('1e100'))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: range() integer start argument expected, got Decimal.
msg104719 - (view) Author: Alexander Belopolsky (Alexander.Belopolsky) Date: 2010-05-01 14:48
On Sat, May 1, 2010 at 3:50 AM, Mark Dickinson <report@bugs.python.org> wrote:
>
> Mark Dickinson <dickinsm@gmail.com> added the comment:
>
> Alexander: range *does* still accept such arguments (in 2.7);  just not floats:
>
>>>> from decimal import Decimal
>>>> range(Decimal(20), Decimal(20))
> []

Decimal must be a special case.  With the code attached by OP and
trunk:80673, I get

$ ./python.exe bad_range.py
...
Traceback (most recent call last):
  File "bad_range.py", line 12, in <module>
    print range(MyInt(2**64), MyInt(2**64+10))
TypeError: range() integer start argument expected, got instance.

Same with new style MyInt:

$ ./python.exe bad_range1.py
...
Traceback (most recent call last):
  File "bad_range1.py", line 12, in <module>
    print range(MyInt(2**64), MyInt(2**64+10))
TypeError: range() integer start argument expected, got MyInt.
msg104720 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2010-05-01 14:59
Decimal is behaving in exactly the same way as MyInt, isn't it? What do you get for range(MyInt(20), MyInt(20))?
msg104728 - (view) Author: Alexander Belopolsky (Alexander.Belopolsky) Date: 2010-05-01 15:35
On Sat, May 1, 2010 at 10:59 AM, Mark Dickinson <report@bugs.python.org> wrote:
>
> Mark Dickinson <dickinsm@gmail.com> added the comment:
>
> Decimal is behaving in exactly the same way as MyInt, isn't it?
> What do you get for range(MyInt(20), MyInt(20))?
>

Hmm, maybe there is a 2.7 bug here after all:

[20, 21, 22]
>>> range(MyInt(2**64), MyInt(2**64+3))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: range() integer start argument expected, got instance.

Same with Decimal:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: range() integer start argument expected, got Decimal.
msg104737 - (view) Author: Robert Bradshaw (robertwb) Date: 2010-05-01 19:02
Thank you Alexander. Yes, there is still an issue for large operands, and the attached patch does fix it. Floats are explicitly checked for and rejected by PyArg_ParseTuple for the "l" format (as called by builtin_range) so to preserve this behavior we can explicitly check in the argument parsing of handle_range_longs as well.

This all goes away in Py3 due to the unification of int and long. (And I agree that using __index__ rather than __int__ fits better there).
msg104741 - (view) Author: Alexander Belopolsky (Alexander.Belopolsky) Date: 2010-05-01 19:20
Robert,

Your patch segfaults on Lib/test/test_builtin.py, but I agree with the
approach.  I'll see if it is easy to fix.
msg104742 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2010-05-01 19:31
> Hmm, maybe there is a 2.7 bug here after all:

Yes, indeed!  Which is why I was suggesting 'wont fix' rather than 'out of date' :)

The bltinmodule2.diff patch from Robert causes a segfault in test_builtin on my system;  I haven't looked at the patch itself properly, but there's likely some refcounting problem.

The patch also lacks tests.
msg104743 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2010-05-01 20:03
A couple of suggestions regarding the patch:

(1) I'd suggest leaving the first part of handle_range_longs intact, up to and including the "/* ilow and ihigh correct now; do istep */" block.  Then build out the three "if (!PyInt_Check(...)) ..." blocks below to include argument conversion.  I think the patch would look cleaner this way.

(2) Rather than using PyNumber_Long, I'd prefer an explicit check for, and call to, nb_int.  This is the behaviour that's used for the 'l' getargs format.  PyNumber_Long is considerably more complicated, and involves looking at __trunc__ and __long__;  so if you use PyNumber_Long you'll still end up with inconsistent behaviour between small and large arguments.
msg104744 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2010-05-01 20:12
I am attaching a patch which does not crash or leak (as tested with -R :) and adds a unit test for OP's case.

The problem, however is that it does not work if new style classes are used in the test case.  See disabled (with if 0) test in the patch.

I'll give it some more thought.
msg104745 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2010-05-01 20:15
Mark,

I did not see your last comment before uploading the last patch.  I think your suggestion to bypass PyNumber_Long will fix the new/old style classes discrepancy.
msg104747 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2010-05-01 20:21
Alexander, I think it should be as simple as replacing the if (!PyInt_Check(ilow) && ...) block with something like this:

	if (!PyInt_Check(ilow) && !PyLong_Check(ilow)) {
		PyNumberMethods *nb = Py_TYPE(ilow)->tp_as_number;
		PyObject *temp;
		if (PyFloat_Check(ilow) || nb == NULL || nb->nb_int == NULL) {
			PyErr_Format(PyExc_TypeError,
				     "range() integer start argument expected, got %s.",
				     ilow->ob_type->tp_name);
			goto Fail;
		}
		temp = (*nb->nb_int)(ilow);
		Py_DECREF(ilow);
		ilow = temp;
		if (temp == NULL)
			goto Fail;
	}

and then doing the same for the ihigh and istep blocks.  But I haven't tested this.

Mark
msg104758 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2010-05-02 00:03
This patch, bltinmodule4.diff, seems to work for all cases.

Mark,

It looks like I came up with almost the same logic as you did.   The only difference that I can see is that your code does not check that nb_int returns an integer.  Also I put repeated logic into a separate function.

The patch includes unit tests that pass reference leak test.
msg104771 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2010-05-02 08:37
Thanks---the new patch looks good.  Pulling the argument conversion out into a separate function makes the whole thing much cleaner.

I still have a couple of nits:

 - Please add a comment before get_range_argument indicating
   what it's for.  I'd also consider naming the function something
   more descriptive like 'convert_range_argument' rather than
   'get_range_argument', but I've never been good with names...

 - Good catch about checking the return type of nb_int.  The error
   message should refer to "__int__" though, not "nb_int":  "nb_int"
   won't make much sense to most Python users.

 - I notice that get_range_argument steals a reference to arg.  That's
   fine of course, but it's a little bit unusual, so there should be
   a comment pointing that out somewhere.  It *might* be preferable to
   not steal the reference, and just do the usual 'return a new
   reference' thing instead; I know that leads to a little bit
   more boilerplate in handle_range_longs, but I think the code ends
   up clearer, and there won't be surprises for someone who tries to
   reuse the code in get_range_argument for their own conversions.  I
   leave it up to you. :)

 - Please could you also add a test for small arguments for each test
   class?

 - Style nit:  the codebase mostly avoids assignments in 'if' conditions;
   please separate the assignment and the NULL test in lines like
   "if (!(ilow = get_range_argument(ilow, "start")))".  Also,
   "if (ilow == NULL)" is preferable to "if (!ilow)".

Thanks again for doing this.
msg104772 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2010-05-02 09:06
Thinking about it a bit more, I really would prefer get_range_argument not to steal a reference.  If I'm reading a bit of C code and encounter something like:

  obj = transform(obj);
  if (obj == NULL) ...

my hindbrain immediately starts fretting that something's wrong, and I have to go and ferret out the definition of 'transform' to be sure.  In contrast, patterns like:

  temp = transform(obj);
  Py_DECREF(obj);
  obj = temp;
  if (obj == NULL) ...

are so common and familiar in the Python codebase that they don't raise the same sort of red flag.
msg104775 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2010-05-02 09:44
BTW, I've changed the various "nb_int should return int object" error messages in Objects/intobject.c to the more meaningful and accurate message: "__int__ method should return an integer", in r80695.
msg104811 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2010-05-03 03:43
Attached patch, issue1533.diff, simplifies reference acrobatics at the expense of extra local variables.  For the first time the first compilation did not have reference counting errors!
msg104812 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2010-05-03 03:46
On Sun, May 2, 2010 at 4:37 AM, Mark Dickinson <report@bugs.python.org> wrote:
..
>  - Please could you also add a test for small arguments for each test
>   class?

Working on it ...
msg104813 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2010-05-03 03:52
>  - Please could you also add a test for small arguments for each test
>   class?

Done.
msg104923 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2010-05-04 11:27
Thanks for the fixes.  The latest patch looks good to me.

Alexander, is it okay for me to commit this?
msg104928 - (view) Author: Alexander Belopolsky (Alexander.Belopolsky) Date: 2010-05-04 12:30
On May 4, 2010, at 7:27 AM, Mark Dickinson <report@bugs.python.org>  
wrote:

>
> Mark Dickinson <dickinsm@gmail.com> added the comment:
>
> Thanks for the fixes.  The latest patch looks good to me.
>
> Alexander, is it okay for me to commit this?
>

Sure. Why do you have to ask?
msg104929 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2010-05-04 12:33
Hi Alexander,

I took the liberty of messing with your patch slightly;  I didn't want to ask you to make further changes since the patch was fine, and my messing was mostly just to satisfy my own fussiness (only the first two items were really necessary):

Minor updates:
 - add Misc/NEWS entry
 - remove trailing whitespace and fix spaces that should have been a tab
 - added some extra tests to check all possible combinations of bad
   arguments, purely to check for refcounting problems
 - initialize low to NULL, to match the Py_XDECREF(low) (could change
   that Py_XDECREF to Py_DECREF instead, but the code's more resistant
   to random refactorings this way --- see next item :)
 - randomly refactor:  regroup blocks for ease of reading
 - don't do PyLong_FromLong(1) until it's needed ('zero' is different,
   since it's always used in the non-error case)
 - [micro-optimization]: don't pass a known zero value to
   get_range_long_argument

Any objections or comments?  Can I apply this version of the patch?
msg104930 - (view) Author: Alexander Belopolsky (Alexander.Belopolsky) Date: 2010-05-04 12:41
I see. Let me take a look.

BTW, I tried to use tabs for indentation in the patch, but emacs was  
not always happy about it. Is there some c-mode setting that I don't  
know about that would take care of that?

On May 4, 2010, at 8:34 AM, Mark Dickinson <report@bugs.python.org>  
wrote:

>
> Mark Dickinson <dickinsm@gmail.com> added the comment:
>
> Hi Alexander,
>
> I took the liberty of messing with your patch slightly;  I didn't  
> want to ask you to make further changes since the patch was fine,  
> and my messing was mostly just to satisfy my own fussiness (only the  
> first two items were really necessary):
>
> Minor updates:
> - add Misc/NEWS entry
> - remove trailing whitespace and fix spaces that should have been a  
> tab
> - added some extra tests to check all possible combinations of bad
>   arguments, purely to check for refcounting problems
> - initialize low to NULL, to match the Py_XDECREF(low) (could change
>   that Py_XDECREF to Py_DECREF instead, but the code's more resistant
>   to random refactorings this way --- see next item :)
> - randomly refactor:  regroup blocks for ease of reading
> - don't do PyLong_FromLong(1) until it's needed ('zero' is different,
>   since it's always used in the non-error case)
> - [micro-optimization]: don't pass a known zero value to
>   get_range_long_argument
>
> Any objections or comments?  Can I apply this version of the patch?
>
> ----------
> Added file: http://bugs.python.org/file17200/issue1533_metd.diff
>
> _______________________________________
> Python tracker <report@bugs.python.org>
> <http://bugs.python.org/issue1533>
> _______________________________________
msg104931 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2010-05-04 12:43
Re emacs:

C-c . python

should set a python 2.x-friendly indentation mode.

There's also a python-new style floating around somewhere on the web (not part of emacs as standard), suitable for the 4-space indent style that's supposed to be used for new code.
msg104948 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2010-05-04 15:11
On Tue, May 4, 2010 at 8:34 AM, Mark Dickinson <report@bugs.python.org> wrote:
..
> I took the liberty of messing with your patch slightly;  I didn't want
> to ask you to make further changes since the patch was fine, and my
> messing was mostly just to satisfy my own fussiness (only the first
> two items were really necessary):
>

Thank you for doing it.  The patch is good to go, questions and comments below are just for my education.

> Minor updates:
>  - add Misc/NEWS entry

What is the standard practice on this?  I thought Misc/NEWS entry was similar to commit message and would be written by a committer.  What are the guidelines for writing such entries?  I see that some entries simply repeat the title of the issues while others got into greater details.

>  - remove trailing whitespace and fix spaces that should have been
> a tab

I've looked at my patch through cat -et and couldn't find any tab/space issues.  I did notice one empty line with a single space that you removed.  Do you use an automated checking tool for this?  I just did grep " $".  BTW, the current trunk version (r80752) of Python/bltinmodule.c has two lines with trailing white space:

$ cat -n Python/bltinmodule.c | grep " $" | cat -et
   641^I^I^IPyErr_SetString(PyExc_TypeError, $
  2966^I        $


>  - added some extra tests to check all possible combinations of bad
>   arguments, purely to check for refcounting problems

With arguments processing segregated in a common function, I am not sure  whether additional tests actually increase coverage.  This brings a question: what is the recommended way to produce coverage statistics for  C modules?  regrtest.py --coverage seems to be for python modules only. 

>  - initialize low to NULL, to match the Py_XDECREF(low) (could change
>   that Py_XDECREF to Py_DECREF instead, but the code's more resistant
>   to random refactorings this way --- see next item :)

Good catch.  Wouldn't the same argument apply to ilow?  Wouldn't static code checkers complain about redundant initialization?

>  - randomly refactor:  regroup blocks for ease of reading

I actually disagree that your regrouping makes the code clearer.  In my version, all idiosyncratic argument processing is done with borrowed references first followed by common processing in three similar blocks.  This, however, is purely matter of taste.  Note that I considered changing i* names to raw* names, but decided not to introduce more changes than necessary.  In your grouping, however, the similarity of variable names is more of an issue.  This said, I don't really have any problem with your choice.

>  - don't do PyLong_FromLong(1) until it's needed ('zero' is different,
>   since it's always used in the non-error case)

Yes, I considered that. A further micro-optimization would be to initialize a static variable in module initialization and reuse it in get_len_of_range_longs as well.  I decided to put it next to zero instead to simplify the logic.

>  - [micro-optimization]: don't pass a known zero value to
>   get_range_long_argument

Is it really worth it?  Default start is probably not that common in case of long arguments.


> Any objections or comments?  Can I apply this version of the patch?
>

The above are not objections.  Please apply this version of the patch.
msg104953 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2010-05-04 16:14
[Some of the Alexander's questions about procedures aren't really related to this issue;  I've answered those offline.  Here are the answers to the others.]


>>  - initialize low to NULL, to match the Py_XDECREF(low) (could change
>>   that Py_XDECREF to Py_DECREF instead, but the code's more resistant
>>   to random refactorings this way --- see next item :)
>
> Good catch.  Wouldn't the same argument apply to ilow?  Wouldn't static code checkers complain about redundant initialization?

ilow doesn't need to be initialized because the PyArgs_ParseTuple is
guaranteed to either fail or initialize it, and I can't see that the
PyArgs_ParseTuple call is likely to change.  But I admit that the lack
of initialization here also makes me uncomfortable, especially in
combination with the assert that's there.  I might add an
initialization.

Do static code checkers really complain about redundant
initializations?  If anything, it seems like good defensive
programming practice to initialize variables, even
unnecessarily---later refactoring might make those initializations
necessary.

>
>>  - randomly refactor:  regroup blocks for ease of reading
>
> I actually disagree that your regrouping makes the code clearer.  In my version, all idiosyncratic argument processing is done with borrowed references first followed by common processing in three similar blocks.  This, however, is purely matter of taste.  Note that I considered changing i* names to raw* names, but decided not to introduce more changes than necessary.  In your grouping, however, the similarity of variable names is more of an issue.  This said, I don't really have any problem with your choice.

Okay, fair enough.  I agree it's a matter of taste.  I like the three
separate blocks, one for each argument, especially since the
refcounting semantics are clear:  each block adds exactly one
reference.  But each to his own. :)

>
>>  - don't do PyLong_FromLong(1) until it's needed ('zero' is different,
>>   since it's always used in the non-error case)
>
> Yes, I considered that. A further micro-optimization would be to initialize a static variable in module initialization and reuse it in get_len_of_range_longs as well.  I decided to put it next to zero instead to simplify the logic.

Hmm.  Possibly.  I have an unhealthy and probably irrational aversion
to non-constant static variables, even if the only time that the
variable is changed is at module initialization.

>
>>  - [micro-optimization]: don't pass a known zero value to
>>   get_range_long_argument
>
> Is it really worth it?  Default start is probably not that common in case of long arguments.

Yes, possibly not. :)  Partly I made this change because the
assignment 'ilow = zero;' again raises a red flag for me, because it's
not accompanied by a 'Py_INCREF(zero);' as I'd expect it to be.  I
realize that in this case it works out (because ilow is already a
borrowed reference, and we're replacing it with a borrowed reference
to zero), but it's sufficiently unusual that I have to think about it.
 This is personal preference again, I guess.
msg104956 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2010-05-04 16:32
Applied to trunk in r80758.

Do people want this to go into 2.6 as well?  The patch would need to be modified to produce a warning for floats instead of giving a TypeError (and the tests would need to be modified to test for that warning).
msg104957 - (view) Author: Alexander Belopolsky (Alexander.Belopolsky) Date: 2010-05-04 16:36
On Tue, May 4, 2010 at 12:32 PM, Mark Dickinson <report@bugs.python.org> wrote:
>
> Mark Dickinson <dickinsm@gmail.com> added the comment:
>
> Applied to trunk in r80758.
>
> Do people want this to go into 2.6 as well?

Also, should additional unit tests forward ported to 3.x?  If so, let
me do it as an exercise in creating a ready to commit patch. :-)
msg104960 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2010-05-04 16:43
+1 for forward-porting/adapting relevant tests to py3k.
msg104970 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2010-05-04 18:30
I am attaching a py3k patch that adds new tests.  Since there are no end user visible changes, I don't believe a Misc/NEWS entry is needed.  A commit message may read:

Issue #1533: Tests only. Added tests for consistency in range function argument processing. 

Note that the first chunk:

-        # Reject floats when it would require PyLongs to represent.
-        # (smaller floats still accepted, but deprecated)
+        # Reject floats.
+        self.assertRaises(TypeError, range, 1., 1., 1.)

is applicable to the trunk as well.
msg105107 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2010-05-05 22:49
Perfect!  Committed in r80836 (py3k);  fixed that one test and comment in r80842 in trunk.

Alexander, do you want to tackle the 2.6 backport?

BTW, I think in most cases it's unnecessary to add Python 3.3 to the Versions field above, since there's no corresponding svn branch for 3.3.  (But it's useful if there's a task that's specifically aimed at 3.3 and not earlier versions---e.g. if something's been deprecated in 3.2 and needs to be removed in 3.3.)
msg105109 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2010-05-05 22:51
That should have been r80839, not r80842.
msg105110 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2010-05-05 23:35
> Alexander, do you want to tackle the 2.6 backport?

I've never done a maintenance branch backport, but here is my attempt:

1. Checkout release26-maint
2. Apply 80757:80758 diff, fix rejected NEWS patch
3. Ignore 80838:80839 diff - small floats are accepted in 2.6 range.
4. Replace small float with large float in bad argument tests.  
5. make; make test; make patchcheck

I could probably use svn merge instead of svn diff + patch.  Did I miss anything important?

BTW, I've discovered "make patchcheck", does it check C files white space issues or only python files?

Mark,

I've sent you two emails off the tracker, but it looks like they did not make it through your spam filters.
msg105129 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2010-05-06 07:59
> I've never done a maintenance branch backport, but here is my attempt:
[...]

Yes, that sounds about right.  But after all that, you'll still need to modify the patch somewhat, since the requirements are different for 2.6:  floats should give a DeprecationWarning rather than a TypeError.  I think that's a straightforward change for Python/bltinmodule.c.  The trickier bit is coming up with tests that work properly---i.e., check that the appropriate warnings are produced, *and* that the the appropriate values are returned.  Look into the 'catch_warnings' function in the warnings module; (there's also 'check_warnings' in test_support, but I think that doesn't exist in 2.6).

'make patchcheck' only checks Python files and ReST files, as far as I can tell.

[I got your off-tracker emails;  will respond anon.]
msg105152 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2010-05-06 17:42
> the requirements are different for 2.6:  floats should give a 
> DeprecationWarning rather than a TypeError. 

I thought about it, but the comment in test_builtin.py,

	# Reject floats when it would require PyLongs to represent.                                                                                                                                          
	# (smaller floats still accepted, but deprecated)  

convinced me that raising TypeError on large floats is a feature.  I don't have a strong opinion on this issue, but I think a conservative approach is not to change current behavior in the maintenance branch unless it is clearly a bug.

I did add a test checking that "smaller floats still accepted, but deprecated."
msg105199 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2010-05-07 13:12
Yes, okay---that makes some sense;  I'm happy to leave floats as they are (i.e., DeprecationWarning for small floats; TypeError for larger floats) and just fix use of __int__ for non-floats.

I'll look at the patch.
msg105200 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2010-05-07 13:26
The backport looks fine.  Applied in r80917.  Thanks, Alexander.
History
Date User Action Args
2010-05-07 13:26:30mark.dickinsonsetstatus: open -> closed
resolution: accepted -> fixed
messages: + msg105200

stage: patch review -> resolved
2010-05-07 13:12:49mark.dickinsonsetassignee: belopolsky -> mark.dickinson
messages: + msg105199
2010-05-06 17:42:31belopolskysetfiles: - issue1533-release26-maint.diff
2010-05-06 17:42:20belopolskysetfiles: + issue1533-release26-maint.diff

messages: + msg105152
2010-05-06 07:59:13mark.dickinsonsetmessages: + msg105129
2010-05-05 23:39:21belopolskysetkeywords: + 26backport
versions: - Python 3.1, Python 2.7, Python 3.2, Python 3.3
2010-05-05 23:35:14belopolskysetfiles: + issue1533-release26-maint.diff
nosy: - Alexander.Belopolsky
messages: + msg105110

2010-05-05 22:51:37mark.dickinsonsetmessages: + msg105109
2010-05-05 22:51:07mark.dickinsonsetmessages: - msg105108
2010-05-05 22:50:37mark.dickinsonsetmessages: + msg105108
2010-05-05 22:49:36mark.dickinsonsetmessages: + msg105107
2010-05-04 18:30:17belopolskysetfiles: + issue1533-py3k-tests.diff

messages: + msg104970
versions: + Python 3.1, Python 3.2, Python 3.3
2010-05-04 16:43:18mark.dickinsonsetassignee: mark.dickinson -> belopolsky
messages: + msg104960
2010-05-04 16:36:45Alexander.Belopolskysetmessages: + msg104957
2010-05-04 16:32:54mark.dickinsonsetmessages: + msg104956
2010-05-04 16:14:40mark.dickinsonsetmessages: + msg104953
2010-05-04 15:12:11belopolskysetassignee: belopolsky -> mark.dickinson
2010-05-04 15:11:17belopolskysetmessages: + msg104948
2010-05-04 12:43:47mark.dickinsonsetmessages: + msg104931
2010-05-04 12:41:26Alexander.Belopolskysetmessages: + msg104930
2010-05-04 12:34:03mark.dickinsonsetfiles: + issue1533_metd.diff

messages: + msg104929
2010-05-04 12:30:44Alexander.Belopolskysetnosy: + Alexander.Belopolsky
messages: + msg104928
2010-05-04 11:27:10mark.dickinsonsetresolution: accepted
messages: + msg104923
2010-05-03 03:52:08belopolskysetfiles: - issue1533.diff
2010-05-03 03:52:00belopolskysetfiles: + issue1533.diff

messages: + msg104813
2010-05-03 03:46:52belopolskysetmessages: + msg104812
2010-05-03 03:44:09belopolskysetfiles: - bltinmodule4.diff
2010-05-03 03:44:02belopolskysetfiles: - bltinmodule3.diff
2010-05-03 03:43:56belopolskysetfiles: - bltinmodule.diff
2010-05-03 03:43:47belopolskysetfiles: + issue1533.diff

messages: + msg104811
2010-05-02 09:44:20mark.dickinsonsetmessages: + msg104775
2010-05-02 09:06:12mark.dickinsonsetmessages: + msg104772
2010-05-02 08:37:40mark.dickinsonsetmessages: + msg104771
2010-05-02 00:06:27belopolskysetfiles: - bltinmodule4.diff
2010-05-02 00:06:20belopolskysetfiles: + bltinmodule4.diff
2010-05-02 00:03:20belopolskysetfiles: + bltinmodule4.diff

messages: + msg104758
stage: test needed -> patch review
2010-05-01 20:21:13mark.dickinsonsetmessages: + msg104747
2010-05-01 20:15:34belopolskysetmessages: + msg104745
2010-05-01 20:12:22belopolskysetfiles: + bltinmodule3.diff

messages: + msg104744
2010-05-01 20:03:08mark.dickinsonsetmessages: + msg104743
2010-05-01 19:33:28mark.dickinsonsetstatus: closed -> open
resolution: out of date -> (no value)
2010-05-01 19:31:40mark.dickinsonsetmessages: + msg104742
2010-05-01 19:24:17belopolskysetassignee: belopolsky
nosy: + belopolsky, - Alexander.Belopolsky
2010-05-01 19:20:52Alexander.Belopolskysetmessages: + msg104741
2010-05-01 19:02:40robertwbsetfiles: + bltinmodule2.diff

messages: + msg104737
2010-05-01 15:35:55Alexander.Belopolskysetmessages: + msg104728
2010-05-01 14:59:37mark.dickinsonsetmessages: + msg104720
2010-05-01 14:48:10Alexander.Belopolskysetmessages: + msg104719
2010-05-01 07:50:09mark.dickinsonsetmessages: + msg104695
2010-05-01 04:58:29loewissetstatus: open -> closed
resolution: wont fix -> out of date
2010-05-01 02:01:18Alexander.Belopolskysetstatus: pending -> open
nosy: + Alexander.Belopolsky, - belopolsky
messages: + msg104687
2010-05-01 01:05:57mark.dickinsonsetstatus: open -> pending
resolution: wont fix
messages: + msg104682
2010-05-01 00:53:38mark.dickinsonsetmessages: + msg104681
stage: test needed
2010-05-01 00:34:52mark.dickinsonsetmessages: + msg104680
versions: - Python 3.1, Python 3.2
2010-02-10 20:24:56mark.dickinsonsetnosy: + mark.dickinson
2010-02-09 16:39:25brian.curtinsetpriority: normal
type: crash -> behavior
versions: + Python 3.1, Python 3.2, - Python 3.0
2009-01-02 21:50:18rhettingersetassignee: rhettinger -> (no value)
2008-12-10 08:52:59loewissetnosy: + loewis
messages: + msg77509
versions: - Python 2.5, Python 2.5.3
2008-11-29 22:53:58akitadasetmessages: + msg76630
versions: + Python 2.6, Python 3.0, Python 2.7, Python 2.5.3
2008-11-29 20:45:36robertwbsetmessages: + msg76624
2008-11-29 20:28:15akitadasetnosy: + akitada
messages: + msg76623
2008-02-25 17:52:53robertwbsetmessages: + msg62989
2008-02-24 23:08:45belopolskysetfiles: + bltinmodule.diff
keywords: + patch
messages: + msg62950
2008-02-24 22:14:51christian.heimessetmessages: + msg62947
2008-02-24 22:00:36belopolskysetmessages: + msg62945
2008-02-24 21:44:36belopolskysetmessages: + msg62943
2008-02-24 21:07:59christian.heimessetnosy: + christian.heimes
messages: + msg62938
2008-02-24 21:03:21belopolskysetnosy: + belopolsky
messages: + msg62937
2008-02-24 13:19:42zanellasetmessages: + msg62898
2008-02-21 17:02:08robertwbsetmessages: + msg62631
2008-02-21 11:16:28zanellasetnosy: + zanella
messages: + msg62617
2007-12-01 13:25:53rhettingersetassignee: rhettinger
nosy: + rhettinger
2007-12-01 01:42:03robertwbsetmessages: + msg58040
2007-12-01 01:37:18josmsetnosy: + josm
messages: + msg58039
2007-12-01 00:28:10robertwbcreate