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.

Author nadeem.vawda
Recipients MizardX, antlong, eric.araujo, nadeem.vawda, niemeyer, pitrou, rhettinger, wrobell, xuanji
Date 2011-03-13.20:17:10
SpamBayes Score 2.7755576e-16
Marked as misclassified No
Message-id <1300047432.31.0.411958898529.issue5863@psf.upfronthosting.co.za>
In-reply-to
Content
Thanks for the review. I'll try and have an updated patch ready by next weekend.

Regarding your comments:

> Is there any reason it doesn't inherit io.BufferedIOBase?
No, there isn't; I'll fix that in my revised patch.

> Since this is a new start, perhaps we should add
>    #define PY_SSIZE_T_CLEAN
> before including "Python.h"?
Sounds like a good idea.

> Just a nit, but I'm not sure there's any point in renaming "the bz2 library"
> to "libbzip2"?
> (also, under Windows I'm not sure the library naming convention is the same
> as under Unix)
Well, the official name for the library is "libbzip2" <bzip.org>. I thought that
the "lib" prefix would make it clearer that the error is referring to the that
library and not _bz2module.c. But if you think it would be better not to make
this change, I'll leave it out.

>> Modules/_bz2module.c:78: "Unrecognized error from libbzip2: %d", bzerror);
> Out of curiousity, did you encounter this condition?
No, I was just programming defensively (in case the underlying library adds
more error codes in future). Unlikely, but I would think it's better than
taking the risk of silently ignoring an error.

> Do note that avail_in and avail_out are 32-bit ints, and therefore this is
> not 64-bit clean. I guess you're just copying the old code here, but that
> would deserve a separate patch later. Perhaps add a comment in the meantime.
Good catch. I'll make a note of it. This would only be a problem for avail_in,
though. The output buffer never grows by more than BIGCHUNK (512KiB) at a time
(see grow_buffer()) so there is no risk of overflowing in avail_out.

> The Windows build files probably need updating as well. Can you do it?
> Otherwise I'll have a try.
I'll give it a try, and let you know if I can't get it to work.
History
Date User Action Args
2011-03-13 20:17:12nadeem.vawdasetrecipients: + nadeem.vawda, rhettinger, niemeyer, pitrou, wrobell, eric.araujo, MizardX, antlong, xuanji
2011-03-13 20:17:12nadeem.vawdasetmessageid: <1300047432.31.0.411958898529.issue5863@psf.upfronthosting.co.za>
2011-03-13 20:17:10nadeem.vawdalinkissue5863 messages
2011-03-13 20:17:10nadeem.vawdacreate