Issue 5734: Fix BufferedRWPair - Python tracker (original) (raw)

Created on 2009-04-11 10:41 by bquinlan, last changed 2022-04-11 14:56 by admin. This issue is now closed.

Messages (8)
msg85849 - (view)	Author: Brian Quinlan (bquinlan) *	Date: 2009-04-11 10:41
The C implementation: - doesn't correctly initialize its reader and writer instances - incorrectly maps its "readinto" method to another class - incorrectly delegates its "closed" property to its base class i.e. this class can't be used at all The Python implementation: - Calls internal methods of its constructor arguments that aren't part of the IOBase interface to determine if its streams are readable/writable There aren't any useful tests for either.
msg85850 - (view)	Author: Antoine Pitrou (pitrou) *	Date: 2009-04-11 10:46
Hey, thanks a lot for caring. I'll take a look at the patch and come back later. If you want to add some more tests, you are welcome too :-)
msg85870 - (view)	Author: Brian Quinlan (bquinlan) *	Date: 2009-04-11 17:50
Hey Antoine, Will do - but first I'll finish up my reason for needing a working version of this class in the first place ;-) Cheers, Brian
msg86090 - (view)	Author: Antoine Pitrou (pitrou) *	Date: 2009-04-17 20:59
I've uploaded the patch for code review here: http://codereview.appspot.com/40126
msg86091 - (view)	Author: Antoine Pitrou (pitrou) *	Date: 2009-04-17 21:11
Reviewers: report_bugs.python.org, Message: Here are some comments. In general, I think the changes to _pyio.py are unwarranted (even though I dislike the naming of _checkReadable and _checkWritable). http://codereview.appspot.com/40126/diff/1/2 File Lib/_pyio.py (left): http://codereview.appspot.com/40126/diff/1/2#oldcode370 Line 370: def _checkReadable(self, msg=None): Not sure why you're removing it. Currently it's used in Lib/socket.py. http://codereview.appspot.com/40126/diff/1/2#oldcode384 Line 384: def _checkWritable(self, msg=None): Same question as for _checkReadable(). http://codereview.appspot.com/40126/diff/1/3 File Lib/test/test_io.py (right): http://codereview.appspot.com/40126/diff/1/3#newcode1121 Line 1121: self.assertTrue(pair.readable) This is probably `pair.readable()` and not `pair.readable`. http://codereview.appspot.com/40126/diff/1/3#newcode1125 Line 1125: self.assertTrue(pair.writable) Same comment as for readable above. http://codereview.appspot.com/40126/diff/1/3#newcode1126 Line 1126: There should probably be a test for seekable() as well. http://codereview.appspot.com/40126/diff/1/4 File Modules/_io/bufferedio.c (right): http://codereview.appspot.com/40126/diff/1/4#newcode1876 Line 1876: Py_DECREF(self->reader); You must use Py_CLEAR so that there is no double free when calling BufferedRWPair_dealloc(). Please review this at http://codereview.appspot.com/40126 Affected files: M Lib/_pyio.py M Lib/test/test_io.py M Modules/_io/bufferedio.c
msg86107 - (view)	Author: Brian Quinlan (bquinlan) *	Date: 2009-04-18 07:41
http://codereview.appspot.com/40126/diff/1/2 File Lib/_pyio.py (left): http://codereview.appspot.com/40126/diff/1/2#oldcode370 Line 370: def _checkReadable(self, msg=None): On 2009/04/17 21:11:15, Antoine Pitrou wrote: > Not sure why you're removing it. Currently it's used in Lib/socket.py. I didn't see the other usages. I removed it because it was only used twice in this file and one of the usages involved an instance other than self i.e. calling an internal method on another instance. Ditto for _checkWriteable Now restored. http://codereview.appspot.com/40126/diff/1/3 File Lib/test/test_io.py (right): http://codereview.appspot.com/40126/diff/1/3#newcode1121 Line 1121: self.assertTrue(pair.readable) On 2009/04/17 21:11:15, Antoine Pitrou wrote: > This is probably `pair.readable()` and not `pair.readable`. Done. http://codereview.appspot.com/40126/diff/1/3#newcode1125 Line 1125: self.assertTrue(pair.writable) On 2009/04/17 21:11:15, Antoine Pitrou wrote: > Same comment as for readable above. Done. http://codereview.appspot.com/40126/diff/1/3#newcode1126 Line 1126: On 2009/04/17 21:11:15, Antoine Pitrou wrote: > There should probably be a test for seekable() as well. Now you are getting greedy. Done. http://codereview.appspot.com/40126/diff/1/4 File Modules/_io/bufferedio.c (right): http://codereview.appspot.com/40126/diff/1/4#newcode1876 Line 1876: Py_DECREF(self->reader); On 2009/04/17 21:11:15, Antoine Pitrou wrote: > You must use Py_CLEAR so that there is no double free when calling > BufferedRWPair_dealloc(). Done. http://codereview.appspot.com/40126
msg86150 - (view)	Author: Antoine Pitrou (pitrou) *	Date: 2009-04-18 23:55
The patch looks ok, thanks.
msg86151 - (view)	Author: Antoine Pitrou (pitrou) *	Date: 2009-04-19 00:11
Committed in r71736.