Issue �3080: Fix import machinery for non-ASCII module names and unencodable paths

[haypo_gmail, 2011-01-21 02:06:36]

http://codereview.appspot.com/3972045/diff/1/Python/import.c
File Python/import.c (right):

http://codereview.appspot.com/3972045/diff/1/Python/import.c#newcode3556
Python/import.c:3556: if (!PyArg_ParseTuple(args, "s:init_builtin", &name))
Oops, test_importlib crashs because of this bug: it should use "U" format, not
"s"

[sasha, 2011-01-21 02:19:14]

Some initial comments.

http://codereview.appspot.com/3972045/diff/1/Python/import.c
File Python/import.c (right):

http://codereview.appspot.com/3972045/diff/1/Python/import.c#newcode122
Python/import.c:122: 'c', 'p', 'y', 't', 'h', 'o', 'n', '-', '3', '2', '\0'};
It is unfortunate that you need to duplicate the same data in wide an narrow
chars.  Can't you remove TAG and CACHEDIR now?

http://codereview.appspot.com/3972045/diff/1/Python/import.c#newcode336
Python/import.c:336: /* XXX: can the previous line fail? */
Whitespace change?

http://codereview.appspot.com/3972045/diff/1/Python/import.c#newcode628
Python/import.c:628: _PyImport_FixupBuiltin(PyObject *mod, const char *name)
Please leave non-essential changes out of this patch.  Added const and
s/filename/nameobj/ are not essential.

http://codereview.appspot.com/3972045/diff/1/Python/import.c#newcode635
Python/import.c:635: res = _PyImport_FixupExtensionUnicode(mod, nameobj,
nameobj);
Does _PyImport_FixupExtensionUnicode still need 3 arguments?

http://codereview.appspot.com/3972045/diff/1/Python/import.c#newcode641
Python/import.c:641: _PyImport_FindExtensionUnicode(PyObject *name, PyObject
*filename)
What is the difference between name and filename now?

http://codereview.appspot.com/3972045/diff/1/Python/import.c#newcode654
Python/import.c:654: mod = PyImport_AddModuleUnicode(name);
I believe elsewhere the naming convention was Foo(char *) -> FooObject(PyObject
*), not FooUnicode(PyObject *).

http://codereview.appspot.com/3972045/diff/1/Python/import.c#newcode684
Python/import.c:684: _PyImport_FindBuiltin(const char *name)
Non-essential changes here.

http://codereview.appspot.com/3972045/diff/1/Python/import.c#newcode920
Python/import.c:920: if (len + Py_UNICODE_strlen(CACHEDIR_UNICODE) +
Py_UNICODE_strlen(PYC_TAG_UNICODE) + 5 > buflen)
CACHEDIR_UNICODE and PYC_TAG_UNICODE are static arrays.  You can use sizeof to
find their sizes.

http://codereview.appspot.com/3972045/diff/1/Python/import.c#newcode1003
Python/import.c:1003: buf[len] = '.'; len++;
buf[len++] = ... is more idiomatic C.

[haypo_gmail, 2011-01-21 17:23:59]

http://codereview.appspot.com/3972045/diff/1/Python/import.c
File Python/import.c (right):

http://codereview.appspot.com/3972045/diff/1/Python/import.c#newcode122
Python/import.c:122: 'c', 'p', 'y', 't', 'h', 'o', 'n', '-', '3', '2', '\0'};
On 2011/01/21 02:19:14, sasha wrote:
> It is unfortunate that you need to duplicate the same data in wide an narrow
> chars.  Can't you remove TAG and CACHEDIR now?

For TAG: PyImport_GetMagicTag() returns a static byte strings.

For CACHEDIR: PyUnicode_FromFormatV() has no format for Py_UNICODE* string.
There is an issue somewhere for that, but I am unable to find it :-)

http://codereview.appspot.com/3972045/diff/1/Python/import.c#newcode336
Python/import.c:336: /* XXX: can the previous line fail? */
On 2011/01/21 02:19:14, sasha wrote:
> Whitespace change?

tabs suck :-)

http://codereview.appspot.com/3972045/diff/1/Python/import.c#newcode628
Python/import.c:628: _PyImport_FixupBuiltin(PyObject *mod, const char *name)
On 2011/01/21 02:19:14, sasha wrote:
> Please leave non-essential changes out of this patch.  Added const and
> s/filename/nameobj/ are not essential.

It don't think that it hurts. I needed const to ensure that the function doesn't
modify name string content.

http://codereview.appspot.com/3972045/diff/1/Python/import.c#newcode635
Python/import.c:635: res = _PyImport_FixupExtensionUnicode(mod, nameobj,
nameobj);
On 2011/01/21 02:19:14, sasha wrote:
> Does _PyImport_FixupExtensionUnicode still need 3 arguments?

_PyImport_LoadDynamicModule() (importdl.c) calls it with 3 different arguments.
Builtins are a special case.

http://codereview.appspot.com/3972045/diff/1/Python/import.c#newcode641
Python/import.c:641: _PyImport_FindExtensionUnicode(PyObject *name, PyObject
*filename)
On 2011/01/21 02:19:14, sasha wrote:
> What is the difference between name and filename now?

As before, I didn't change the meaning of these variables. name is the module
name, filename is the module filename...

http://codereview.appspot.com/3972045/diff/1/Python/import.c#newcode654
Python/import.c:654: mod = PyImport_AddModuleUnicode(name);
On 2011/01/21 02:19:14, sasha wrote:
> I believe elsewhere the naming convention was Foo(char *) ->
FooObject(PyObject
> *), not FooUnicode(PyObject *).

I don't know this convention. What does it change? It gives less readable source
code?

http://codereview.appspot.com/3972045/diff/1/Python/import.c#newcode684
Python/import.c:684: _PyImport_FindBuiltin(const char *name)
On 2011/01/21 02:19:14, sasha wrote:
> Non-essential changes here.

It is important that name string content is not changed.

http://codereview.appspot.com/3972045/diff/1/Python/import.c#newcode920
Python/import.c:920: if (len + Py_UNICODE_strlen(CACHEDIR_UNICODE) +
Py_UNICODE_strlen(PYC_TAG_UNICODE) + 5 > buflen)
On 2011/01/21 02:19:14, sasha wrote:
> CACHEDIR_UNICODE and PYC_TAG_UNICODE are static arrays.  You can use sizeof to
> find their sizes.

We need a PY_ARRAY_SIZE() function for that.

http://codereview.appspot.com/3972045/diff/1/Python/import.c#newcode1003
Python/import.c:1003: buf[len] = '.'; len++;
On 2011/01/21 02:19:14, sasha wrote:
> buf[len++] = ... is more idiomatic C.

Sorry, I don't like C and I prefer to do only one thing per instruction.

[sasha, 2011-01-21 18:21:04]

http://codereview.appspot.com/3972045/diff/1/Python/import.c
File Python/import.c (right):

http://codereview.appspot.com/3972045/diff/1/Python/import.c#newcode122
Python/import.c:122: 'c', 'p', 'y', 't', 'h', 'o', 'n', '-', '3', '2', '\0'};
On 2011/01/21 17:24:00, haypo_gmail wrote:
> On 2011/01/21 02:19:14, sasha wrote:
> > It is unfortunate that you need to duplicate the same data in wide an narrow
> > chars.  Can't you remove TAG and CACHEDIR now?
> 
> For TAG: PyImport_GetMagicTag() returns a static byte strings.
> 
> For CACHEDIR: PyUnicode_FromFormatV() has no format for Py_UNICODE* string.
> There is an issue somewhere for that, but I am unable to find it :-)

In this case, why do you need static *_UNICODE variants?  It seems to me
converting from bytes at run-time is cheap both in terms of code and CPU cycles.

http://codereview.appspot.com/3972045/diff/1/Python/import.c#newcode628
Python/import.c:628: _PyImport_FixupBuiltin(PyObject *mod, const char *name)
On 2011/01/21 17:24:00, haypo_gmail wrote:
> On 2011/01/21 02:19:14, sasha wrote:
> > Please leave non-essential changes out of this patch.  Added const and
> > s/filename/nameobj/ are not essential.
> 
> It don't think that it hurts. I needed const to ensure that the function
doesn't
> modify name string content.

You can commit cosmetic changes separately.  I know this is not an option during
RC phase, but it is hard enough to keep track of real char* to PyObject*
changes, so anything that can simplify the review will be appreciated.

http://codereview.appspot.com/3972045/diff/1/Python/import.c#newcode654
Python/import.c:654: mod = PyImport_AddModuleUnicode(name);
On 2011/01/21 17:24:00, haypo_gmail wrote:
> On 2011/01/21 02:19:14, sasha wrote:
> > I believe elsewhere the naming convention was Foo(char *) ->
> FooObject(PyObject
> > *), not FooUnicode(PyObject *).
> 
> I don't know this convention. What does it change? It gives less readable
source
> code?

Didn't you introduce it yourself with PyModule_GetFilenameObject?  Yes, it would
probably be more readable to call it PyModule_GetFilenameUnicode.  It is likely
that there will be more changes like this, so it would be nice to get some
consistency.

http://codereview.appspot.com/3972045/diff/1/Python/import.c#newcode684
Python/import.c:684: _PyImport_FindBuiltin(const char *name)
On 2011/01/21 17:24:00, haypo_gmail wrote:
> On 2011/01/21 02:19:14, sasha wrote:
> > Non-essential changes here.
> 
> It is important that name string content is not changed.

Traditionally, Python code base has been rather lax on const-correctness.  I am
usually +0 on adding const where appropriate, but here it is a distraction for
the reviewers.  When I see signature change in your patch, I expect a char* to
PyObject* change and it is a distraction when it is just an added const.

http://codereview.appspot.com/3972045/diff/1/Python/import.c#newcode920
Python/import.c:920: if (len + Py_UNICODE_strlen(CACHEDIR_UNICODE) +
Py_UNICODE_strlen(PYC_TAG_UNICODE) + 5 > buflen)
On 2011/01/21 17:24:00, haypo_gmail wrote:
> On 2011/01/21 02:19:14, sasha wrote:
> > CACHEDIR_UNICODE and PYC_TAG_UNICODE are static arrays.  You can use sizeof
to
> > find their sizes.
> 
> We need a PY_ARRAY_SIZE() function for that.

Well, sizeof(a)/sizeof(*a) is not that hard to type, but a macro is a good idea.

http://codereview.appspot.com/3972045/diff/1/Python/import.c#newcode1003
Python/import.c:1003: buf[len] = '.'; len++;
On 2011/01/21 17:24:00, haypo_gmail wrote:
> On 2011/01/21 02:19:14, sasha wrote:
> > buf[len++] = ... is more idiomatic C.
> 
> Sorry, I don't like C and I prefer to do only one thing per instruction.

"I don't like C" is not a good excuse.    I don't know what you mean by "one
thing per instruction," but I find two statements per line hard to follow. 
Maybe this whole function can be reduced to a call to  PyUnicode_FromFormat?

[sasha, 2011-01-21 18:25:57]

http://codereview.appspot.com/3972045/diff/1/Include/import.h
File Include/import.h (right):

http://codereview.appspot.com/3972045/diff/1/Include/import.h#newcode13
Include/import.h:13: char *name,                 /* UTF-8 encoded string */
Thanks for documenting char* arguments like this.  I wonder if we can convince
Georg that these are documentation changes and should go in RC2?

[sasha, 2011-01-21 19:27:48]

http://codereview.appspot.com/3972045/diff/1/Include/moduleobject.h
File Include/moduleobject.h (right):

http://codereview.appspot.com/3972045/diff/1/Include/moduleobject.h#newcode16
Include/moduleobject.h:16: PyAPI_FUNC(PyObject *) PyModule_NewUnicode(PyObject
*name);
"Unicode" suffix leaves it ambiguous whether name is PyObject or Py_UNICODE.  It
is also not clear whether it refers to the argument or the return value.  The
most descriptive name would be PyModule_NewFromUnicodeObject, but this may be
too long.

http://codereview.appspot.com/3972045/diff/1/Include/pycapsule.h
File Include/pycapsule.h (right):

http://codereview.appspot.com/3972045/diff/1/Include/pycapsule.h#newcode52
Include/pycapsule.h:52: const char *name,           /* UTF-8 encoded string */
Please separate these changes to a separate patch/commit.   I like these
comments, but they make this patch too big to fit in my brain. :-)

http://codereview.appspot.com/3972045/diff/1/Modules/zipimport.c
File Modules/zipimport.c (right):

http://codereview.appspot.com/3972045/diff/1/Modules/zipimport.c#newcode210
Modules/zipimport.c:210: subname =
Py_UNICODE_strrchr(PyUnicode_AS_UNICODE(fullname), '.');
This won't work if Py_UNICODE buffer in fullname is not null-terminated or has
embedded '\0'.  (I know that neither is likely.) Maybe PyUnicode_Find() is a
better option here.  Note that single-sharacter ascii strings are cached, so
creating '.' object is cheap.

http://codereview.appspot.com/3972045/diff/1/Modules/zipimport.c#newcode215
Modules/zipimport.c:215: subname++;
The code below is just impenetrable.   Please don't reuse len like this. 
Compiler knows how to allocate registers better than you do.  Better still,

subnamelen = PyUnicode_AS_UNICODE(fullname) + PyUnicode_GET_SIZE(fullname) -
subname;
PyUnicode_FromUnicode(subname, subnamelen);

http://codereview.appspot.com/3972045/diff/1/Modules/zipimport.c#newcode228
Modules/zipimport.c:228: Py_UNICODE path[MAXPATHLEN + 1], *p;
Rather than using a fixed size buffer and low level code, wouldn't it be better
to call PyUnicode_Concat and PyUnicode_Replace?

http://codereview.appspot.com/3972045/diff/1/Modules/zipimport.c#newcode1212
Modules/zipimport.c:1212: goto error;
Do you need two goto labels?  It may be better to just initialize code to NULL. 
Also, I think it is more common in Python codebase to call exit label "done". 
"finally" jumps out as a Python keyword.

[haypo_gmail, 2011-02-22 23:52:37]

On 2011/01/21 18:25:57, sasha wrote:
> http://codereview.appspot.com/3972045/diff/1/Include/import.h
> File Include/import.h (right):
> 
> http://codereview.appspot.com/3972045/diff/1/Include/import.h#newcode13
> Include/import.h:13: char *name,                 /* UTF-8 encoded string */
> Thanks for documenting char* arguments like this.  I wonder if we can convince
> Georg that these are documentation changes and should go in RC2?

I just commited this commit: r88517 (3.3), r88518 (3.2). So it will be in 3.2.1.