py3k lzma patch

patch that adds the lzma module.

[Martin v. Löwis, 2010-10-31 16:33:38]

http://codereview.appspot.com/2724043/diff/1/Doc/library/lzma.rst
File Doc/library/lzma.rst (right):

http://codereview.appspot.com/2724043/diff/1/Doc/library/lzma.rst#newcode54
Doc/library/lzma.rst:54: .. attribute:: lzma.options.check
The attribute is called check, not lzma.options.check.

http://codereview.appspot.com/2724043/diff/1/Modules/lzmamodule.c
File Modules/lzmamodule.c (right):

http://codereview.appspot.com/2724043/diff/1/Modules/lzmamodule.c#newcode27
Modules/lzmamodule.c:27: #define VERSION "0.6.0"
I recommend that this is version 1.0 (or doesn't get a version number at all for
the copy included in Python).

http://codereview.appspot.com/2724043/diff/1/Modules/lzmamodule.c#newcode60
Modules/lzmamodule.c:60: if (!PyThread_acquire_lock(obj->lock, 0)) { \
I recommend to put the acquire/release object lock along with the GIL. I fail to
see why this first acquire operation could possibly fail - the object is
protected by the GIL, right? so the lzma lock should only be necessary if we are
not holding the GIL. That is, assuming liblzma is not thread-safe - if it is,
there is no need for another lock at all.

http://codereview.appspot.com/2724043/diff/1/Modules/lzmamodule.c#newcode228
Modules/lzmamodule.c:228: typedef struct
I find the option handling fairly involved and unreadable. It would be better,
IMO, if as many options as possible would be plain C types. You can use the
structmember support to still make them easily python-accessible. If you keep
PyObject* (which IMO should be really avoided), make sure you have
garbage-collection support for them. The structured value should be flattened,
e.g. by introducing compresslevel_min and compresslevel_max.

http://codereview.appspot.com/2724043/diff/1/Modules/lzmamodule.c#newcode264
Modules/lzmamodule.c:264: lzma_FILE *fp;
I suggest that struct lzma_FILE is inlined here - no need to have a separate
memory block.

http://codereview.appspot.com/2724043/diff/1/Modules/lzmamodule.c#newcode1165
Modules/lzmamodule.c:1165: PyLong_FromUnsignedLongLong(LZMA_FILTER_LZMA2),
This (and all other PyTuple_Pack calls) leak references.

http://codereview.appspot.com/2724043/diff/1/Modules/lzmamodule.c#newcode3501
Modules/lzmamodule.c:3501: {"crc32", (PyCFunction)LZMA_crc32,
Is it really necessary to expose another set of CRC functions from this module?
Who would be using them?

[proyvind, 2010-10-31 20:46:13]

On 2010/10/31 16:33:38, Martin v. Löwis wrote:
> http://codereview.appspot.com/2724043/diff/1/Doc/library/lzma.rst
> File Doc/library/lzma.rst (right):
> 
> http://codereview.appspot.com/2724043/diff/1/Doc/library/lzma.rst#newcode54
> Doc/library/lzma.rst:54: .. attribute:: lzma.options.check
> The attribute is called check, not lzma.options.check.
> 
> http://codereview.appspot.com/2724043/diff/1/Modules/lzmamodule.c
> File Modules/lzmamodule.c (right):
> 
> http://codereview.appspot.com/2724043/diff/1/Modules/lzmamodule.c#newcode27
> Modules/lzmamodule.c:27: #define VERSION "0.6.0"
> I recommend that this is version 1.0 (or doesn't get a version number at all
for
> the copy included in Python).
k, can be dropped :)
> 
> http://codereview.appspot.com/2724043/diff/1/Modules/lzmamodule.c#newcode60
> Modules/lzmamodule.c:60: if (!PyThread_acquire_lock(obj->lock, 0)) { \
> I recommend to put the acquire/release object lock along with the GIL. I fail
to
> see why this first acquire operation could possibly fail - the object is
> protected by the GIL, right? so the lzma lock should only be necessary if we
are
> not holding the GIL. That is, assuming liblzma is not thread-safe - if it is,
> there is no need for another lock at all.
I haven't really put much thought into this one, it's really just ripped off
from bz2module.c...
> 
> http://codereview.appspot.com/2724043/diff/1/Modules/lzmamodule.c#newcode228
> Modules/lzmamodule.c:228: typedef struct
> I find the option handling fairly involved and unreadable. It would be better,
> IMO, if as many options as possible would be plain C types. You can use the
> structmember support to still make them easily python-accessible. If you keep
> PyObject* (which IMO should be really avoided), make sure you have
> garbage-collection support for them. The structured value should be flattened,
> e.g. by introducing compresslevel_min and compresslevel_max.
Yeah, it's certainly a bit awkward, I've so not really had much luck receiving
any of the feedback requested before now. I guess I should review the code and
give it a bit more attention again. :)
> 
> http://codereview.appspot.com/2724043/diff/1/Modules/lzmamodule.c#newcode264
> Modules/lzmamodule.c:264: lzma_FILE *fp;
> I suggest that struct lzma_FILE is inlined here - no need to have a separate
> memory block.
Not sure what you mean..? Moving the definition within LZMAFileObject would seem
rather tedious IMO, but I'm probably not fully grasping what you mean..?
> 
> http://codereview.appspot.com/2724043/diff/1/Modules/lzmamodule.c#newcode1165
> Modules/lzmamodule.c:1165: PyLong_FromUnsignedLongLong(LZMA_FILTER_LZMA2),
> This (and all other PyTuple_Pack calls) leak references.
k, will fix
> 
> http://codereview.appspot.com/2724043/diff/1/Modules/lzmamodule.c#newcode3501
> Modules/lzmamodule.c:3501: {"crc32", (PyCFunction)LZMA_crc32,
> Is it really necessary to expose another set of CRC functions from this
module?
> Who would be using them?
The crc32 function of liblzma is more optimized than the one that comes with
zlib, which is why I'm exposing it.
IMO it should be preferred for binascii to use as well when available.

[Martin v. Löwis, 2010-10-31 21:21:28]

>> Modules/lzmamodule.c:264: lzma_FILE *fp;
>> I suggest that struct lzma_FILE is inlined here - no need to have a
> separate
>> memory block.
> Not sure what you mean..? Moving the definition within LZMAFileObject
> would seem rather tedious IMO, but I'm probably not fully grasping what
> you mean..?

I think LZMAFileObject should go like this:

...
  char* f_bufptr;
  unsigned char buf[1<<15];
  lzma_stream strm;
  FILE *fp;
  int encoding;
  int eof;
  lzma_filter filters[LZMA_FILTERS_MAX + 1];

and then lzma_close/lzma_read/lzma_write should just expect
an LZMAFileObject, instead of an lzma_FILE. lzma_open would not
allocate lzma_file anymore, but be passed an LZMAFileObject as
well.

Why would that be tedious?

That leaves the issue aside that I think the module shouldn't be using
stdio in the first place.

[proyvind, 2010-10-31 21:32:17]

On 2010/10/31 21:21:28, Martin v. Löwis wrote:
> >> Modules/lzmamodule.c:264: lzma_FILE *fp;
> >> I suggest that struct lzma_FILE is inlined here - no need to have a
> > separate
> >> memory block.
> > Not sure what you mean..? Moving the definition within LZMAFileObject
> > would seem rather tedious IMO, but I'm probably not fully grasping what
> > you mean..?
> 
> I think LZMAFileObject should go like this:
> 
> ...
>   char* f_bufptr;
>   unsigned char buf[1<<15];
>   lzma_stream strm;
>   FILE *fp;
>   int encoding;
>   int eof;
>   lzma_filter filters[LZMA_FILTERS_MAX + 1];
> 
> and then lzma_close/lzma_read/lzma_write should just expect
> an LZMAFileObject, instead of an lzma_FILE. lzma_open would not
> allocate lzma_file anymore, but be passed an LZMAFileObject as
> well.
> 
> Why would that be tedious?
Ah, never mind, I was thinking in a totally different way, makes sense now. :)
> 
> That leaves the issue aside that I think the module shouldn't be using
> stdio in the first place.
I guess I'll end up rewriting it in a totally different not using it at all
anyways.. :p