classification
Title: detach() implementation
Type: enhancement Stage:
Components: IO Versions: Python 3.1
process
Status: closed Resolution: accepted
Dependencies: Superseder:
Assigned To: benjamin.peterson Nosy List: amaury.forgeotdarc, benjamin.peterson, pitrou
Priority: normal Keywords: patch

Created on 2009-04-29 22:19 by benjamin.peterson, last changed 2009-05-01 20:41 by benjamin.peterson. This issue is now closed.

Files
File name Uploaded Description Edit
detach.patch benjamin.peterson, 2009-04-29 22:19
detach.patch benjamin.peterson, 2009-04-29 23:33
detach.patch benjamin.peterson, 2009-04-30 21:46
Messages (8)
msg86830 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2009-04-29 22:19
Here's the detach() implementation for BufferedIOBase and TextIOBase.
msg86844 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2009-04-30 15:51
Reviewers: ,

Please review this at http://codereview.appspot.com/52075

Affected files:
   Doc/library/io.rst
   Lib/_pyio.py
   Lib/test/test_io.py
   Modules/_io/bufferedio.c
   Modules/_io/textio.c
msg86845 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2009-04-30 16:04
http://codereview.appspot.com/52075/diff/1/2
File Doc/library/io.rst (right):

http://codereview.appspot.com/52075/diff/1/2#newcode366
Line 366: Disconnect this buffer from its underlying raw stream and
return it.
This sentence is a bit ambiguous.
How about “Separate the underlying raw stream from the
:class:`BufferedIOBase` and return it” ?

Also, you should mention that some implementations will raise
io.UnsupportedOperation if the operation doesn't make sense.

http://codereview.appspot.com/52075/diff/1/2#newcode605
Line 605: in an unusable state.
You should mention that some implementations will raise
io.UnsupportedOperation if the operation doesn't make sense.

http://codereview.appspot.com/52075/diff/1/3
File Lib/_pyio.py (right):

http://codereview.appspot.com/52075/diff/1/3#newcode836
Line 836: return self
Uh, this doesn't really make sense. Better let it raise
io.UnsupportedOperation.

http://codereview.appspot.com/52075/diff/1/4
File Lib/test/test_io.py (right):

http://codereview.appspot.com/52075/diff/1/4#newcode532
Line 532: self.assertIs(buf.detach(), raw)
Perhaps a test of what happens when calling detach() a second time?
(it should probably raise a ValueError)

http://codereview.appspot.com/52075/diff/1/4#newcode1504
Line 1504: self.assertIs(t.detach(), b)
Same comment as for buffered binary tests.

http://codereview.appspot.com/52075/diff/1/5
File Modules/_io/bufferedio.c (right):

http://codereview.appspot.com/52075/diff/1/5#newcode470
Line 470: Py_CLEAR(self->raw);
Why not the simpler:
     raw = self->raw;
     self->raw = NULL;
?

http://codereview.appspot.com/52075/diff/1/5#newcode1547
Line 1547: self->detached = 1;
This should be 0.

http://codereview.appspot.com/52075/diff/1/6
File Modules/_io/textio.c (right):

http://codereview.appspot.com/52075/diff/1/6#newcode1080
Line 1080: "raw stream has been detached"); \
"underlying buffer" rather than "raw stream"?

http://codereview.appspot.com/52075/diff/1/6#newcode1092
Line 1092: "raw stream has been detached"); \
same as above

http://codereview.appspot.com/52075/diff/1/6#newcode1112
Line 1112: Py_CLEAR(self->buffer);
Why not the simpler:
     buffer = self->buffer;
     self->buffer = NULL;
?

http://codereview.appspot.com/52075
msg86858 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2009-04-30 21:46
Thanks for the review! New patch attached...
msg86868 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2009-05-01 08:06
The various structures now contain several 'boolean' flags. It would 
improve memory usage to use bitfields. Or would this impact performance 
?
msg86877 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2009-05-01 10:35
> The various structures now contain several 'boolean' flags. It would 
> improve memory usage to use bitfields. Or would this impact performance 
> ?

Those structures usually allocate a 4KB- or 8KB- buffer each, so I don't
think squeezing two or three bytes would do a lot of difference
(performance probably wouldn't be affected either).

Also, I don't find the bitfield notation very readable (and it isn't
found anywhere else in the interpreter AFAIK), but that's just me :-)
msg86883 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2009-05-01 12:57
The new patch looks good.
msg86908 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2009-05-01 20:41
Applied in r72175.
History
Date User Action Args
2009-05-01 20:41:21benjamin.petersonsetstatus: open -> closed
resolution: accepted
messages: + msg86908
2009-05-01 12:57:17pitrousetmessages: + msg86883
2009-05-01 10:35:33pitrousetmessages: + msg86877
2009-05-01 08:06:39amaury.forgeotdarcsetnosy: + amaury.forgeotdarc
messages: + msg86868
2009-04-30 21:46:15benjamin.petersonsetfiles: + detach.patch

messages: + msg86858
2009-04-30 16:04:48pitrousetmessages: + msg86845
2009-04-30 15:51:37pitrousetmessages: + msg86844
2009-04-29 23:33:33benjamin.petersonsetfiles: + detach.patch
2009-04-29 22:19:48benjamin.petersoncreate