classification
Title: itertools.tee uses int for indices
Type: enhancement Stage: resolved
Components: Versions: Python 3.4
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: rhettinger Nosy List: pitrou, python-dev, rhettinger, serhiy.storchaka
Priority: low Keywords: patch

Created on 2013-09-19 09:21 by pitrou, last changed 2013-09-20 21:14 by rhettinger. This issue is now closed.

Files
File name Uploaded Description Edit
tee_64b.patch pitrou, 2013-09-19 11:37 review
Messages (10)
msg198049 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-09-19 09:21
itertools.tee uses ints rather than Py_ssize_t for indices. Using Py_ssize_t would not make the object bigger, since other fields will already be pointer-aligned.
msg198059 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-09-19 11:37
Simple patch attached.
msg198097 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-09-19 17:33
I don't think we need Py_ssize_t for integers from 0 to 57.
msg198099 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-09-19 17:57
> I don't think we need Py_ssize_t for integers from 0 to 57.

IMO it's better to use Py_ssize_t there, since it can get combined with
other Py_ssize_t values. Avoiding spurious conversions eliminates a
common source of errors. We don't gain anything by keeping it as an int.
msg198100 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-09-19 18:05
We gain stability.

I doubt we should change this. However in any case you forgot about tee_reduce().
msg198103 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-09-19 18:07
> I doubt we should change this.

Currently itertools.tee() will break if more than 2**31 elements are
saved in the inner structures. I certainly think it should be fixed,
rather than bite someone by surprise one day.

> However in any case you forgot about tee_reduce().

Will take a look.
msg198109 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-09-19 18:24
Ah, my bad, it seems I misunderstood the tee() implementation. So apparenly even index cannot get past 57. It's more of a consistency patch then, and I agree it isn't very important.
msg198135 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-09-20 09:06
There are disadvantages in the changing int to Py_ssize_t. Converting Py_ssize_t  to/from Python int is a little harder than converting C int or long. This change (as any other change) has a risk of introduce new bugs (as you can see on example of your patch).

I suggest just add (yet one) explicit comment.

    int index; /* 0 <= index <= LINKCELLS */
msg198137 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-09-20 09:34
> I suggest just add (yet one) explicit comment.
> 
>     int index; /* 0 <= index <= LINKCELLS */

Ok, I think a comment is good enough here.
msg198165 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2013-09-20 20:19
New changeset 900bf633b7f4 by Antoine Pitrou in branch 'default':
Add a comment making it explicit that itertools.tee() is already 64bit-safe (issue #19049)
http://hg.python.org/cpython/rev/900bf633b7f4
History
Date User Action Args
2013-09-20 21:14:34rhettingersetassignee: rhettinger
2013-09-20 20:19:45pitrousetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2013-09-20 20:19:29python-devsetnosy: + python-dev
messages: + msg198165
2013-09-20 09:34:16pitrousetmessages: + msg198137
2013-09-20 09:06:32serhiy.storchakasetmessages: + msg198135
2013-09-19 18:24:07pitrousetpriority: normal -> low
versions: - Python 3.3
title: itertools.tee isn't 64-bit compliant -> itertools.tee uses int for indices
messages: + msg198109

type: behavior -> enhancement
stage: needs patch -> patch review
2013-09-19 18:07:35pitrousetmessages: + msg198103
2013-09-19 18:05:19serhiy.storchakasetmessages: + msg198100
2013-09-19 17:57:21pitrousetmessages: + msg198099
2013-09-19 17:33:00serhiy.storchakasetmessages: + msg198097
2013-09-19 11:37:17pitrousetfiles: + tee_64b.patch
keywords: + patch
messages: + msg198059
2013-09-19 09:21:08pitroucreate