This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: fix bug 678519: cStringIO self iterator
Type: Stage:
Components: Extension Modules Versions: Python 2.3
process
Status: closed Resolution: accepted
Dependencies: Superseder:
Assigned To: Nosy List: mbrierst, nnorwitz, rhettinger
Priority: normal Keywords: patch

Created on 2003-03-01 19:49 by mbrierst, last changed 2022-04-10 16:07 by admin. This issue is now closed.

Files
File name Uploaded Description Edit
patchcstrio mbrierst, 2003-03-01 19:50
patchcstrio2 mbrierst, 2003-03-05 22:16
patchcstrio3 mbrierst, 2003-03-19 18:39
patchcstrio4 mbrierst, 2003-03-19 21:17
Messages (10)
msg42927 - (view) Author: Michael Stone (mbrierst) Date: 2003-03-01 19:49
StringIO.StringIO already appears to be
a self-iterator.  This patch makes cStringIO.StringIO
a self-iterator as well.

It also does a tiny bit of cleanup to cStringIO.
msg42928 - (view) Author: Michael Stone (mbrierst) Date: 2003-03-05 22:16
Logged In: YES 
user_id=670441

patchcstrio2 is a better version, more
cleaned up.  Use it instead.
msg42929 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2003-03-12 01:44
Logged In: YES 
user_id=80475

I don't know about the other reviewers but I prefer that the 
patches be attached to the original bug instead on a new 
patch tracker on SF.  This makes it easier to follow the 
dialogue on this issue.
msg42930 - (view) Author: Michael Stone (mbrierst) Date: 2003-03-12 02:35
Logged In: YES 
user_id=670441

I prefer that too, but I can't attach patches to
existing bug reports in sourceforge, only
to bug reports or patches I open myself.
Nor can I delete patches I have attached
if I don't like them.

Actually, the advice I read somewhere or
other (python.org developer faq?) recommends
opening a separate patch all the time, but
I'd rather be able to put them with the bug reports.

I used to paste patches directly into the text
of a message, but this is only good for extremely
short patches on sourceforge.  When doing that
I noticed that patches for old bugs that haven't
been discussed in a few months tend to get ignored,
which is another plus for opening a separate patch.
(There seem to be several very old bugs which
have solutions attached or discussion indicates they
should be closed)
msg42931 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2003-03-18 00:55
Logged In: YES 
user_id=80475

I'm going to unassign this one because the patch makes 
me uncomfortable.  The tp_iter slot was already filled in a 
way that is reasonable and the new code doesn't seem to 
be an improvement.

If you go ahead with it, carefully consider whether some 
negative effects can arise from eliminating the 
get/setattrs.  Also, the call to readline should avoid 
creating a new empty tuple on each call (either make a 
single one and re-use it everytime or alter readline to 
accept a NULL for args).

The 0,0,0,0,0,0,0 style in the type definition should be 
spelled-out line by line so that it is maintainable and is 
consistent with other modules.

All that being said, the test cases were nice and code runs 
flawlessly.  
msg42932 - (view) Author: Michael Stone (mbrierst) Date: 2003-03-19 18:39
Logged In: YES 
user_id=670441

I'm not sure I understand your concern with the new
tp_iter slot, it just makes cStringIO a self iterator
as requested on python-dev, going for
the analogy with file objects, right?  Actually it should
probably use the still-being-debated GenericGetIter
or whatever it will be called, but not until the debate is
over.

I think the get/setattrs are okay.  Everything they
did is done by the default get/set attrs, once we
set up the appropriate methods and members
(there's just the one member, softspace).  I thought
replacing them by the defaults would be clearer
and easier to maintain.  Also, it is in analogy with
fileobject.c, so I thought making the cStringIO
implementation more like file's would be good.

As for the creating a new tuple every time and
the 0,0,0,0 style, you're absolutely right, I've attached
a new patch that fixes those up per your suggestions.
I was creating a new tuple every time in analogy
with iterobject.c's calliter_iternext.  Perhaps that
should be changed as well?
msg42933 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2003-03-19 19:54
Logged In: YES 
user_id=80475

It looks good to me, compiles okay, passes tests, etc.  I do 
prefer that you get one more reviewer to look at it.  Neal or 
MvL might be a good choice.

GvR picked PyObject_SelfIter to be the name of the 
iterator's tp_iter slot filler.  So you can go ahead and use it 
to eliminate IO_getiter.

One nit, when you load the next patch, copy in the 
unchanged lines from the original.  There are many lines 
marked as having a change but the content is the same.  
This means that something changed in the whitespace.  
It's not big deal but it makes the patch harder to review.



msg42934 - (view) Author: Michael Stone (mbrierst) Date: 2003-03-19 21:17
Logged In: YES 
user_id=670441

Okay, patchstrio4 uses PyObject_SelfIter and
doesn't have as much of my prettification, so
there aren't any whitespace-only diff lines. (I think)
Should I assign this patch to either Neal or MvL
for further review, or would that be impolite?
msg42935 - (view) Author: Neal Norwitz (nnorwitz) * (Python committer) Date: 2003-03-19 21:27
Logged In: YES 
user_id=33168

I don't think it's impolite.  I'll try to take a look later,
unless someone beats me to it. :-)
msg42936 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2003-04-24 15:51
Logged In: YES 
user_id=80475

Committed as:

Modules\cStringIO.c 2.40
Lib\test\test_StringIO.py 1.15
History
Date User Action Args
2022-04-10 16:07:16adminsetgithub: 38078
2003-03-01 19:49:27mbrierstcreate