Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(119)

Issue 40126: [issue5734] Fix BufferedRWPair (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
11 years, 6 months ago by Antoine Pitrou
Modified:
11 years, 3 months ago
Reviewers:
brian1, report
Base URL:
http://svn.python.org/view/*checkout*/python/branches/py3k/
Visibility:
Public.

Patch Set 1 #

Total comments: 11
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
Antoine Pitrou
Here are some comments. In general, I think the changes to _pyio.py are unwarranted (even ...
11 years, 6 months ago (2009-04-17 21:11:14 UTC) #1
brian1
11 years, 6 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.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b