zipimport.c needs to support namespace packages when no 'directory' entry exists

[berkerpeksag]

http://bugs.python.org/review/14905/diff/5345/Modules/zipimport.c
File Modules/zipimport.c (right):

http://bugs.python.org/review/14905/diff/5345/Modules/zipimport.c#newcode979
Modules/zipimport.c:979: dirnameobj = PyUnicode_Substring(nameobj, 0, i+1);
i + 1

http://bugs.python.org/review/14905/diff/5345/Modules/zipimport.c#newcode991
Modules/zipimport.c:991: t = Py_BuildValue("Nhllnhhl", path, 0, 0, 0, 0, 0, 0,
0);
It would be good to use a better variable name instead of `t`.

http://bugs.python.org/review/14905/diff/5345/Modules/zipimport.c#newcode994
Modules/zipimport.c:994: err = PyDict_SetItem(files, dirnameobj, t);
It would be good to use a better variable name instead of `err`.

http://bugs.python.org/review/14905/diff/5345/Modules/zipimport.c#newcode997
Modules/zipimport.c:997: if (err != 0) {
I prefer this one :)

    if (err == -1) {

See:
http://docs.python.org/dev/c-api/dict.html?highlight=pydict_setitem#PyDict_Se...

[eric.snow]

While no expert in C or the C-API, a few things jumped out at me.  I've noted
these in-line.  The main thing is that each time through the loop you are
creating new objects, but not decref'ing them.  That will be a memory leak.

You should be able to do everything within the for loop and I am not sure that
you need to Py_CLEAR(dirnameobj) every time through the loop.

Anyway, if I understand correctly, this code pulls out the string up to the
first SEP, and adds it to the mapping.  Does it properly handle more than one
level of namespace package (pkg/subpkg/foo.py)?

http://bugs.python.org/review/14905/diff/5345/Modules/zipimport.c
File Modules/zipimport.c (right):

http://bugs.python.org/review/14905/diff/5345/Modules/zipimport.c#newcode862
Modules/zipimport.c:862: PyObject *dirnameobj = NULL;
dirnameobj could be declared at the beginning of the for loop where it gets
used.

http://bugs.python.org/review/14905/diff/5345/Modules/zipimport.c#newcode973
Modules/zipimport.c:973: Py_UCS4 c;
Declarations need to go at the beginning of blocks (see PEP 7).

http://bugs.python.org/review/14905/diff/5345/Modules/zipimport.c#newcode974
Modules/zipimport.c:974: for (i = 0; i < PyUnicode_GetLength(nameobj) - 1; i++)
{
Doesn't the condition get evaluated every loop?  Also, what happens when
PyUnicode_GetLength() fails for some reason?  If you pull the call out before
the loop you can address both thing there.

http://bugs.python.org/review/14905/diff/5345/Modules/zipimport.c#newcode974
Modules/zipimport.c:974: for (i = 0; i < PyUnicode_GetLength(nameobj) - 1; i++)
{
PyUnicode_GetLength() returns the number of code points.  Is that what you want?

http://bugs.python.org/review/14905/diff/5345/Modules/zipimport.c#newcode983
Modules/zipimport.c:983: Py_CLEAR(dirnameobj);
shouldn't the decref happen here?  each time through the loop you're getting a
new reference.

http://bugs.python.org/review/14905/diff/5345/Modules/zipimport.c#newcode988
Modules/zipimport.c:988: if (path == NULL)
decref dirnameobj here too.

http://bugs.python.org/review/14905/diff/5345/Modules/zipimport.c#newcode992
Modules/zipimport.c:992: if (t == NULL)
decref dirnameobj here.  Also decref path here.

http://bugs.python.org/review/14905/diff/5345/Modules/zipimport.c#newcode995
Modules/zipimport.c:995: Py_CLEAR(dirnameobj);
Also decref dirnameobj (and path) here.

http://bugs.python.org/review/14905/diff/5345/Modules/zipimport.c#newcode1014
Modules/zipimport.c:1014: Py_XDECREF(dirnameobj);
This decref should probably go inside the loop.

[jerub]

I've responded to the review points. Am uploading a new patch now.

http://bugs.python.org/review/14905/diff/5345/Modules/zipimport.c
File Modules/zipimport.c (right):

http://bugs.python.org/review/14905/diff/5345/Modules/zipimport.c#newcode862
Modules/zipimport.c:862: PyObject *dirnameobj = NULL;
On 2012/08/04 05:03:35, eric.snow wrote:
> dirnameobj could be declared at the beginning of the for loop where it gets
> used.

Done.

http://bugs.python.org/review/14905/diff/5345/Modules/zipimport.c#newcode973
Modules/zipimport.c:973: Py_UCS4 c;
On 2012/08/04 05:03:35, eric.snow wrote:
> Declarations need to go at the beginning of blocks (see PEP 7).

Done.

http://bugs.python.org/review/14905/diff/5345/Modules/zipimport.c#newcode974
Modules/zipimport.c:974: for (i = 0; i < PyUnicode_GetLength(nameobj) - 1; i++)
{
On 2012/08/04 05:03:35, eric.snow wrote:
> Doesn't the condition get evaluated every loop?  Also, what happens when
> PyUnicode_GetLength() fails for some reason?  If you pull the call out before
> the loop you can address both thing there.

Done.

http://bugs.python.org/review/14905/diff/5345/Modules/zipimport.c#newcode979
Modules/zipimport.c:979: dirnameobj = PyUnicode_Substring(nameobj, 0, i+1);
On 2012/08/02 18:47:20, berkerpeksag wrote:
> i + 1

Done.

http://bugs.python.org/review/14905/diff/5345/Modules/zipimport.c#newcode983
Modules/zipimport.c:983: Py_CLEAR(dirnameobj);
On 2012/08/04 05:03:35, eric.snow wrote:
> shouldn't the decref happen here?  each time through the loop you're getting a
> new reference.

A Py_CLEAR is equivilent to a Py_XDECREF and setting the arg to None. I've
changed Py_CLEAR to Py_DECREF here to avoid confusion.

http://bugs.python.org/review/14905/diff/5345/Modules/zipimport.c#newcode988
Modules/zipimport.c:988: if (path == NULL)
On 2012/08/04 05:03:35, eric.snow wrote:
> decref dirnameobj here too.

Done.

http://bugs.python.org/review/14905/diff/5345/Modules/zipimport.c#newcode991
Modules/zipimport.c:991: t = Py_BuildValue("Nhllnhhl", path, 0, 0, 0, 0, 0, 0,
0);
On 2012/08/02 18:47:20, berkerpeksag wrote:
> It would be good to use a better variable name instead of `t`.

Done.

http://bugs.python.org/review/14905/diff/5345/Modules/zipimport.c#newcode992
Modules/zipimport.c:992: if (t == NULL)
On 2012/08/04 05:03:35, eric.snow wrote:
> decref dirnameobj here.  Also decref path here.

Done.

http://bugs.python.org/review/14905/diff/5345/Modules/zipimport.c#newcode994
Modules/zipimport.c:994: err = PyDict_SetItem(files, dirnameobj, t);
On 2012/08/02 18:47:20, berkerpeksag wrote:
> It would be good to use a better variable name instead of `err`.

'err' is the most popular variable name to assign the value of PyDict_SetItem: 
for x in $(grep -rh ' = PyDict_SetItem' *  | cut -f 1 -d=) ; do echo $x; done |
sort | uniq -c  | sort -n
      1 code
      1 i
      1 ok
      1 stat
      1 x
      3 int
      3 rc
      3 result
      6 r
      6 status
     13 res
     21 err

http://bugs.python.org/review/14905/diff/5345/Modules/zipimport.c#newcode995
Modules/zipimport.c:995: Py_CLEAR(dirnameobj);
On 2012/08/04 05:03:35, eric.snow wrote:
> Also decref dirnameobj (and path) here.

Done.

http://bugs.python.org/review/14905/diff/5345/Modules/zipimport.c#newcode997
Modules/zipimport.c:997: if (err != 0) {
On 2012/08/02 18:47:20, berkerpeksag wrote:
> I prefer this one :)
> 
>     if (err == -1) {
> 
> See:
>
http://docs.python.org/dev/c-api/dict.html?highlight=pydict_setitem#PyDict_Se...

Done.

http://bugs.python.org/review/14905/diff/5345/Modules/zipimport.c#newcode1014
Modules/zipimport.c:1014: Py_XDECREF(dirnameobj);
On 2012/08/04 05:03:35, eric.snow wrote:
> This decref should probably go inside the loop.

Done.

[storchaka]

http://bugs.python.org/review/14905/diff/5772/Modules/zipimport.c
File Modules/zipimport.c (right):

http://bugs.python.org/review/14905/diff/5772/Modules/zipimport.c#newcode484
Modules/zipimport.c:484: if (err == -1)
This is an unrelated change.

http://bugs.python.org/review/14905/diff/5772/Modules/zipimport.c#newcode998
Modules/zipimport.c:998: toc_entry = Py_BuildValue("Ohllnhhl", path, 0, 0, 0, 0,
0, 0, 0);
Use "N" format for path and don't decref path.

http://bugs.python.org/review/14905/diff/5772/Modules/zipimport.c#newcode1004
Modules/zipimport.c:1004: err = PyDict_SetItem(files, dirnameobj, toc_entry);
I think you can factorize some code (PyUnicode_FromFormat + Py_BuildValue +
PyDict_SetItem) to the separate function.

[eric.smith]

http://bugs.python.org/review/14905/diff/5772/Modules/zipimport.c
File Modules/zipimport.c (right):

http://bugs.python.org/review/14905/diff/5772/Modules/zipimport.c#newcode973
Modules/zipimport.c:973: 
Some comments here about why this loop exists would be helpful. If I stumbled
across this code, it would take me a while to figure out what it's doing.

In fact, maybe moving this entire inner loop out into a function
add_missing_dirs(files, nameobj) (or whatever params are needed) might be a good
idea.

http://bugs.python.org/review/14905/diff/5772/Modules/zipimport.c#newcode979
Modules/zipimport.c:979: Py_UCS4 c = PyUnicode_READ_CHAR(nameobj, i);
I think using PyUnicode_FindChar would be better than looping over every
individual character. Speed probably won't make a difference (and might be
worse), but it would improve readability.

http://bugs.python.org/review/14905/diff/5772/Modules/zipimport.c#newcode998
Modules/zipimport.c:998: toc_entry = Py_BuildValue("Ohllnhhl", path, 0, 0, 0, 0,
0, 0, 0);
On 2013/02/16 23:36:26, storchaka wrote:
> Use "N" format for path and don't decref path.

I think creating a function new_toc_entry(path, ...) would be a good idea.
Having 2 calls to Py_BuildValue that must be in sync is a maintenance problem
waiting to happen.