[issue5863] bz2.BZ2File should accept other file-like objects. (issue4274045)

[Nadeem Vawda]

Nadeem Vawda <nadeem.vawda at gmail.com> added the comment:

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.

----------

_______________________________________
Python tracker <report at bugs.python.org>
<http://bugs.python.org/issue5863>
_______________________________________