[issue5734] Fix BufferedRWPair - Code Review (original) (raw)

Issue 40126: [issue5734] Fix BufferedRWPair (Closed)

Can't Edit Can't Publish+Mail Start Review Created: 16 years, 2 months ago by Antoine Pitrou Modified: 15 years, 11 months ago Reviewers: brian1, report Base URL: http://svn.python.org/view/\*checkout\*/python/branches/py3k/ Visibility: Public.	Patch Set 1# Total comments: 11 Created: 16 years, 2 months ago Download[raw] [tar.bz2] Unified diffs Side-by-side diffs Delta from patch set Stats (+134 lines, -45 lines) Patch M Lib/_pyio.py View 6 chunks +18 lines, -19 lines 3 comments Download M Lib/test/test_io.py View 2 chunks +91 lines, -6 lines 6 comments Download M Modules/_io/bufferedio.c View 6 chunks +25 lines, -20 lines 2 comments Download Messages Total messages: 2 Expand All Messages | Collapse All Messages Antoine Pitrou Here are some comments. In general, I think the changes to _pyio.py are unwarranted (even ... 16 years, 2 months ago (2009-04-17 21:11:14 UTC)#1 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(). Sign in to reply to this message. brian1 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 ... 16 years, 2 months ago (2009-04-18 07:41:30 UTC)#2 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. Sign in to reply to this message. Expand All Messages	Collapse All Messages

Issue 40126: [issue5734] Fix BufferedRWPair (Closed)
Created 16 years, 2 months ago by Antoine Pitrou
Modified 15 years, 11 months ago
Reviewers: report_bugs.python.org, brian1
Base URL: http://svn.python.org/view/\*checkout\*/python/branches/py3k/
Comments: 11