Rewrite Python import machinery to use unicode

Related Python issue: http://bugs.python.org/issue9425

Read also http://mail.python.org/pipermail/python-dev/2010-July/101619.html

[Ezio Melotti, 2010-07-30 00:38:52]

http://codereview.appspot.com/1874048/diff/1/11
File Lib/test/test_import.py (right):

http://codereview.appspot.com/1874048/diff/1/11#newcode298
Lib/test/test_import.py:298: pass
This "else" seems unnecessary.

http://codereview.appspot.com/1874048/diff/1/13
File Lib/test/test_subprocess.py (right):

http://codereview.appspot.com/1874048/diff/1/13#newcode839
Lib/test/test_subprocess.py:839: self.assertEquals(exitcode, 0)
This should be assertEqual.

http://codereview.appspot.com/1874048/diff/1/14
File Lib/test/test_sys.py (left):

http://codereview.appspot.com/1874048/diff/1/14#oldcode513
Lib/test/test_sys.py:513: self.assert_(b"UnicodeEncodeError:" in stderr,
This should use assertIn.

http://codereview.appspot.com/1874048/diff/1/14#oldcode514
Lib/test/test_sys.py:514: "%r not in %s" % (b"UniodeEncodeError:",
ascii(stderr)))
Typo (and the message shouldn't be necessary if assertIn is used).

http://codereview.appspot.com/1874048/diff/1/14
File Lib/test/test_sys.py (right):

http://codereview.appspot.com/1874048/diff/1/14#newcode514
Lib/test/test_sys.py:514: self.assert_(pattern in stderr,
This should use assertIn too.

http://codereview.appspot.com/1874048/diff/1/18
File Modules/getpath.c (right):

http://codereview.appspot.com/1874048/diff/1/18#newcode302
Modules/getpath.c:302: return /* FIXME: return an error */;
These FIXME should be fixed.

http://codereview.appspot.com/1874048/diff/1/19
File Modules/main.c (right):

http://codereview.appspot.com/1874048/diff/1/19#newcode274
Modules/main.c:274: Py_XDECREF(command_bytes);
Isn't Py_DECREF() enough here?

http://codereview.appspot.com/1874048/diff/1/19#newcode752
Modules/main.c:752: } else {
The "else" should be on a new line (see PEP7).

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

http://codereview.appspot.com/1874048/diff/1/20#newcode82
Modules/zipimport.c:82: path = PyBytes_AsString(pathbytes);
Can't you just DECREF pathbytes once here, instead of putting a DECREF in the if
and another after it?

http://codereview.appspot.com/1874048/diff/1/22
File Objects/fileobject.c (right):

http://codereview.appspot.com/1874048/diff/1/22#newcode60
Objects/fileobject.c:60: } else
This else should be a new line and have {} too (see PEP7).

http://codereview.appspot.com/1874048/diff/1/28
File Parser/tokenizer.c (right):

http://codereview.appspot.com/1874048/diff/1/28#newcode477
Parser/tokenizer.c:477: } else
The "else" should be on a new line and use the {} (see PEP7).

http://codereview.appspot.com/1874048/diff/1/30
File Python/ast.c (right):

http://codereview.appspot.com/1874048/diff/1/30#newcode141
Python/ast.c:141: tmp = Py_BuildValue("(NlON)", filename_obj, lineno, Py_None,
loc);
Shouldn't loc be DECREF'ed here?

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

http://codereview.appspot.com/1874048/diff/1/35#newcode726
Python/import.c:726: static PyObject * get_sourcefile(PyObject *file);
Extra space.

http://codereview.appspot.com/1874048/diff/1/39
File Python/sysmodule.c (right):

http://codereview.appspot.com/1874048/diff/1/39#newcode1940
Python/sysmodule.c:1940: myformat(char *name, FILE *fp, const char *format,
va_list va)
Is this function public or should it be _myformat?

http://codereview.appspot.com/1874048/diff/1/39#newcode1980
Python/sysmodule.c:1980: PySys_FormatStderr(const char *format, ...)
Shouldn't this be documented?

[haypo, 2010-07-30 03:45:40]

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

http://codereview.appspot.com/1874048/diff/1/20#newcode82
Modules/zipimport.c:82: path = PyBytes_AsString(pathbytes);
On 2010/07/30 00:38:52, Ezio Melotti wrote:
> Can't you just DECREF pathbytes once here, instead of putting a DECREF in the
if
> and another after it?

No because PyBytes_AsString() just gives a pointer into the PyBytesObject. If
you destroy the object, the pointer is no more valid.

http://codereview.appspot.com/1874048/diff/1/30
File Python/ast.c (right):

http://codereview.appspot.com/1874048/diff/1/30#newcode141
Python/ast.c:141: tmp = Py_BuildValue("(NlON)", filename_obj, lineno, Py_None,
loc);
On 2010/07/30 00:38:52, Ezio Melotti wrote:
> Shouldn't loc be DECREF'ed here?

No, because "N" format borrows a reference the object.

http://codereview.appspot.com/1874048/diff/1/39
File Python/sysmodule.c (right):

http://codereview.appspot.com/1874048/diff/1/39#newcode1940
Python/sysmodule.c:1940: myformat(char *name, FILE *fp, const char *format,
va_list va)
On 2010/07/30 00:38:52, Ezio Melotti wrote:
> Is this function public or should it be _myformat?

"static" means private. I think that the name is correct.

[Antoine Pitrou, 2010-07-30 13:35:47]

There seem to be a lot of FIXMEs waiting for resolution ;)

http://codereview.appspot.com/1874048/diff/1/3
File Include/Python.h (right):

http://codereview.appspot.com/1874048/diff/1/3#newcode131
Include/Python.h:131: /* _Py_char2wchar and _Py_wchar2char lives in main.c */
s/lives/live/

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

http://codereview.appspot.com/1874048/diff/1/5#newcode54
Include/import.h:54: PyAPI_FUNC(PyObject *)_PyImport_FindExtensionUnicode(char
*, PyObject *);
Are the old non-unicode APIs still needed?

http://codereview.appspot.com/1874048/diff/1/9
File Lib/distutils/file_util.py (right):

http://codereview.appspot.com/1874048/diff/1/9#newcode228
Lib/distutils/file_util.py:228: f = open(filename, "w", encoding="utf-8",
errors="surrogateescape")
Why the hardcoding to utf-8?

http://codereview.appspot.com/1874048/diff/1/10
File Lib/platform.py (right):

http://codereview.appspot.com/1874048/diff/1/10#newcode959
Lib/platform.py:959: import subprocess
Isn't there a potential bootstrap problem?

http://codereview.appspot.com/1874048/diff/1/13
File Lib/test/test_subprocess.py (right):

http://codereview.appspot.com/1874048/diff/1/13#newcode841
Lib/test/test_subprocess.py:841: @unittest.skipIf(sysconfig.is_python_build(),
"need an installed Python")
Why this? It works fine right now.

http://codereview.appspot.com/1874048/diff/1/18
File Modules/getpath.c (right):

http://codereview.appspot.com/1874048/diff/1/18#newcode192
Modules/getpath.c:192: return NULL;
Why don't you set errno anymore?

http://codereview.appspot.com/1874048/diff/1/19
File Modules/main.c (right):

http://codereview.appspot.com/1874048/diff/1/19#newcode749
Modules/main.c:749: *bytes++ = c - 0xdc00;
Is this right? How about surrogate pairs if wchar_t is a 16-bit integer type?
What if c is not between 0xdc00 and 0xdcff?

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

http://codereview.appspot.com/1874048/diff/1/20#newcode200
Modules/zipimport.c:200: if (PyUnicode_GET_SIZE(self->prefix) != 0)
What if self->prefix or self->archive is NULL?

http://codereview.appspot.com/1874048/diff/1/21
File Objects/codeobject.c (right):

http://codereview.appspot.com/1874048/diff/1/21#newcode8
Objects/codeobject.c:8: PyObject *_Py_code_object_list = NULL;
You should explain what this is for.

http://codereview.appspot.com/1874048/diff/1/21#newcode49
Objects/codeobject.c:49: if (_Py_code_object_list == NULL)
How about simply using Py_CLEAR()?

http://codereview.appspot.com/1874048/diff/1/23
File Objects/moduleobject.c (right):

http://codereview.appspot.com/1874048/diff/1/23#newcode79
Objects/moduleobject.c:79: err = PyErr_WarnFormat(PyExc_RuntimeWarning, 1,
Is there any point in making this change?

http://codereview.appspot.com/1874048/diff/1/24
File Objects/object.c (right):

http://codereview.appspot.com/1874048/diff/1/24#newcode12
Objects/object.c:12: extern PyObject *_Py_code_object_list;
This should be added to codeobject.h instead.

http://codereview.appspot.com/1874048/diff/1/32
File Python/ceval.c (left):

http://codereview.appspot.com/1874048/diff/1/32#oldcode800
Python/ceval.c:800: /* Make it easier to find out where we are with a debugger
*/
I'm not sure anyone uses it in real-life but I don't see any point in removing
it.

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

http://codereview.appspot.com/1874048/diff/1/35#newcode140
Python/import.c:140: FILE* _Py_fopen(PyObject *unicode, const char *mode);
Non-static functions should probably be declared in Include headers instead.

http://codereview.appspot.com/1874048/diff/1/35#newcode1063
Python/import.c:1063: return NULL;
Lacks an error.

http://codereview.appspot.com/1874048/diff/1/35#newcode1177
Python/import.c:1177: return NULL;
You must free the arena here.

http://codereview.appspot.com/1874048/diff/1/35#newcode1257
Python/import.c:1257: return;
Perhaps you should add an error message if Py_VerboseFlag.

http://codereview.appspot.com/1874048/diff/1/35#newcode1454
Python/import.c:1454: && file[len-3] == '.'
It seems you have completely changed the logic here.
It was originally returning if `file[-4:-1] != ".py"`. Now it returns if
`file[-3:] == ".py"`.

http://codereview.appspot.com/1874048/diff/1/35#newcode1656
Python/import.c:1656: find_module(const char *fullname, const char *name,
PyObject *search_path,
Is there any reason why find_module should still take module names as "char *"
variables names rather that "PyObject *"?

http://codereview.appspot.com/1874048/diff/1/35#newcode1950
Python/import.c:1950: * extension.  name is the module name, also exclusive of
extension.
Can you explain lendelta? Is it the length of the extension?
(then rename it extlen?)

[haypo, 2010-07-30 20:47:04]

Answer to Antoine's comments

http://codereview.appspot.com/1874048/diff/1/3
File Include/Python.h (right):

http://codereview.appspot.com/1874048/diff/1/3#newcode131
Include/Python.h:131: /* _Py_char2wchar and _Py_wchar2char lives in main.c */
On 2010/07/30 13:35:47, Antoine Pitrou wrote:
> s/lives/live/

fixed by r83290

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

http://codereview.appspot.com/1874048/diff/1/5#newcode54
Include/import.h:54: PyAPI_FUNC(PyObject *)_PyImport_FindExtensionUnicode(char
*, PyObject *);
On 2010/07/30 13:35:47, Antoine Pitrou wrote:
> Are the old non-unicode APIs still needed?

_PyImport_FindExtension() is still used but it may be possible to remove it.

I always fear breaking backward compatibility, even if "_Py*" are supposed to be
private functions.

http://codereview.appspot.com/1874048/diff/1/9
File Lib/distutils/file_util.py (right):

http://codereview.appspot.com/1874048/diff/1/9#newcode228
Lib/distutils/file_util.py:228: f = open(filename, "w", encoding="utf-8",
errors="surrogateescape")
On 2010/07/30 13:35:47, Antoine Pitrou wrote:
> Why the hardcoding to utf-8?

utf-8 is maybe not needed, only errors.

http://codereview.appspot.com/1874048/diff/1/10
File Lib/platform.py (right):

http://codereview.appspot.com/1874048/diff/1/10#newcode959
Lib/platform.py:959: import subprocess
I already commited this patch on trunk (r80166) and py3k (r80167), but it broke
the bootstrap on trunk. So I reverted the patch on both branches, even if it
works correctly on py3k (including the bootstrap). See
http://mail.python.org/pipermail/python-checkins/2010-April/092092.html

http://codereview.appspot.com/1874048/diff/1/13
File Lib/test/test_subprocess.py (right):

http://codereview.appspot.com/1874048/diff/1/13#newcode839
Lib/test/test_subprocess.py:839: self.assertEquals(exitcode, 0)
On 2010/07/30 00:38:52, Ezio Melotti wrote:
> This should be assertEqual.

"grep assertEquals Lib/test/test_*py|wc -l" gives 1632 in py3K. My patch doesn't
add a new code to .assertEquals(), it's just refactoring. If it should be fixed,
it should be fixed directly in py3k.

http://codereview.appspot.com/1874048/diff/1/13#newcode841
Lib/test/test_subprocess.py:841: @unittest.skipIf(sysconfig.is_python_build(),
"need an installed Python")
On 2010/07/30 13:35:47, Antoine Pitrou wrote:
> Why this? It works fine right now.

Hum. I supposed that it didn't work some weeks ago, but you are right: it does
now work correctly. Fixed in r83295 (remove skipIf)

http://codereview.appspot.com/1874048/diff/1/18
File Modules/getpath.c (right):

http://codereview.appspot.com/1874048/diff/1/18#newcode192
Modules/getpath.c:192: return NULL;
On 2010/07/30 13:35:47, Antoine Pitrou wrote:
> Why don't you set errno anymore?

_Py_wgetcwd() is a static function and is only called by copy_absolute() which
doesn't use errno. I don't think that errno=ERANGE is revelant / useless here.

http://codereview.appspot.com/1874048/diff/1/19
File Modules/main.c (right):

http://codereview.appspot.com/1874048/diff/1/19#newcode749
Modules/main.c:749: *bytes++ = c - 0xdc00;
On 2010/07/30 13:35:47, Antoine Pitrou wrote:
> Is this right? How about surrogate pairs if wchar_t is a 16-bit integer type?
> What if c is not between 0xdc00 and 0xdcff?

No, it's completly wrong :-) Fixed by r83297

http://codereview.appspot.com/1874048/diff/1/21
File Objects/codeobject.c (right):

http://codereview.appspot.com/1874048/diff/1/21#newcode8
Objects/codeobject.c:8: PyObject *_Py_code_object_list = NULL;
On 2010/07/30 13:35:47, Antoine Pitrou wrote:
> You should explain what this is for.

done in r83298

http://codereview.appspot.com/1874048/diff/1/21#newcode49
Objects/codeobject.c:49: if (_Py_code_object_list == NULL)
On 2010/07/30 13:35:47, Antoine Pitrou wrote:
> How about simply using Py_CLEAR()?

Yep, it is shorter and do the same operations => done in r83298

http://codereview.appspot.com/1874048/diff/1/23
File Objects/moduleobject.c (right):

http://codereview.appspot.com/1874048/diff/1/23#newcode79
Objects/moduleobject.c:79: err = PyErr_WarnFormat(PyExc_RuntimeWarning, 1,
On 2010/07/30 13:35:47, Antoine Pitrou wrote:
> Is there any point in making this change?

I prefer PyErr_WarnFormat() because the caller doesn't have to manage a
temporary buffer (especially the buffer size).

http://codereview.appspot.com/1874048/diff/1/24
File Objects/object.c (right):

http://codereview.appspot.com/1874048/diff/1/24#newcode12
Objects/object.c:12: extern PyObject *_Py_code_object_list;
On 2010/07/30 13:35:47, Antoine Pitrou wrote:
> This should be added to codeobject.h instead.

Right. Done in r83299. Should I use "extern" for a private data? I see
PyAPI_DATA() but it is for *public* data.

http://codereview.appspot.com/1874048/diff/1/32
File Python/ceval.c (left):

http://codereview.appspot.com/1874048/diff/1/32#oldcode800
Python/ceval.c:800: /* Make it easier to find out where we are with a debugger
*/
On 2010/07/30 13:35:47, Antoine Pitrou wrote:
> I'm not sure anyone uses it in real-life but I don't see any point in removing
> it.

The problem is that _PyUnicode_AsString() raises an error if the filename
contains a surrogate character. It should use surrogateescape error handler, or
save/restore the current exception.

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

http://codereview.appspot.com/1874048/diff/1/35#newcode140
Python/import.c:140: FILE* _Py_fopen(PyObject *unicode, const char *mode);
On 2010/07/30 13:35:47, Antoine Pitrou wrote:
> Non-static functions should probably be declared in Include headers instead.

I moved _Py_fopen() declaration into Python.h. I hope that it's not the worst
file, but I don't know the best header for this function. And I marked
_Py_stat() as static.

=> r83300

http://codereview.appspot.com/1874048/diff/1/35#newcode1063
Python/import.c:1063: return NULL;
On 2010/07/30 13:35:47, Antoine Pitrou wrote:
> Lacks an error.

Right. r83301 raises a MemoryError.

http://codereview.appspot.com/1874048/diff/1/35#newcode1177
Python/import.c:1177: return NULL;
On 2010/07/30 13:35:47, Antoine Pitrou wrote:
> You must free the arena here.

Fixed by r83302

http://codereview.appspot.com/1874048/diff/1/35#newcode1257
Python/import.c:1257: return;
On 2010/07/30 13:35:47, Antoine Pitrou wrote:
> Perhaps you should add an error message if Py_VerboseFlag.

Use bytes is a temporary solution. It should use unicode strings and use
CreateDirectoryA() on Windows.

http://codereview.appspot.com/1874048/diff/1/35#newcode1454
Python/import.c:1454: && file[len-3] == '.'
On 2010/07/30 13:35:47, Antoine Pitrou wrote:
> It seems you have completely changed the logic here.
> It was originally returning if `file[-4:-1] != ".py"`. Now it returns if
> `file[-3:] == ".py"`.

Oops, nice catch. Fixed by r83303.

http://codereview.appspot.com/1874048/diff/1/35#newcode1656
Python/import.c:1656: find_module(const char *fullname, const char *name,
PyObject *search_path,
On 2010/07/30 13:35:47, Antoine Pitrou wrote:
> Is there any reason why find_module should still take module names as "char *"
> variables names rather that "PyObject *"?

fullname and (sub)name are ASCII strings (Python module names). It's not
necessary to change their type for my needs (support non encodable bytes in
module directory name), and it would require more changes.

http://codereview.appspot.com/1874048/diff/1/35#newcode1950
Python/import.c:1950: * extension.  name is the module name, also exclusive of
extension.
On 2010/07/30 13:35:47, Antoine Pitrou wrote:
> Can you explain lendelta? Is it the length of the extension?
> (then rename it extlen?)

In py3k (before the patch), len is only used in "new-fangled macintosh (macosx)
or Cygwin" case to extract the directory name. In my patch, "lendelta" looks to
be the length of the extension.

[haypo, 2010-07-30 21:06:33]

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

http://codereview.appspot.com/1874048/diff/1/35#newcode1454
Python/import.c:1454: && file[len-3] == '.'
> `file[-4:-1] != ".py"`
> `file[-3:] == ".py"`

Oooh. I didn't noticed the change != to ==. Fixed by r83304 (this time, I
checked by the unit tests...).

[amaury, 2010-07-31 11:33:01]

The remaining XXX or FIXME should be addressed.
And new public functions should be documented.

http://codereview.appspot.com/1874048/diff/1/9
File Lib/distutils/file_util.py (right):

http://codereview.appspot.com/1874048/diff/1/9#newcode228
Lib/distutils/file_util.py:228: f = open(filename, "w", encoding="utf-8",
errors="surrogateescape")
On 2010/07/30 20:47:04, haypo wrote:
> utf-8 is maybe not needed, only errors.
What is this function used for?

http://codereview.appspot.com/1874048/diff/1/17
File Lib/test/test_xml_etree.py (right):

http://codereview.appspot.com/1874048/diff/1/17#newcode27
Lib/test/test_xml_etree.py:27: # ignore all tests if the current working
directory is undecodable
s/undecodable/unencodable/

http://codereview.appspot.com/1874048/diff/1/19
File Modules/main.c (right):

http://codereview.appspot.com/1874048/diff/1/19#newcode659
Modules/main.c:659: filename_bytes = PyUnicode_EncodeFSDefault(filenameObj);
Can PyUnicode_EncodeFSDefault() fail? In this case, the error should be handled

http://codereview.appspot.com/1874048/diff/1/19#newcode663
Modules/main.c:663: p_cfilename = "<decoding error>";
Here we have an exception set, should PyErr_Clear() be called?
Is there a way to test this code path?

http://codereview.appspot.com/1874048/diff/1/23
File Objects/moduleobject.c (right):

http://codereview.appspot.com/1874048/diff/1/23#newcode192
Objects/moduleobject.c:192: PyModule_GetFilenameObject(PyObject *m)
This function should be documented; especially since it seems to return a
borrowed reference.

http://codereview.appspot.com/1874048/diff/1/27
File PC/import_nt.c (right):

http://codereview.appspot.com/1874048/diff/1/27#newcode18
PC/import_nt.c:18: FILE *PyWin_FindRegisteredModule(const char *moduleName,
can module names be any unicode string? which encoding is used here?

http://codereview.appspot.com/1874048/diff/1/27#newcode22
PC/import_nt.c:22: /* FIXME: use wchar_t* type */
this function should use wchar_t everywhere, RegQueryValueW &co...

http://codereview.appspot.com/1874048/diff/1/28
File Parser/tokenizer.c (right):

http://codereview.appspot.com/1874048/diff/1/28#newcode474
Parser/tokenizer.c:474: stream = PyObject_CallMethod(io, "open", "Osis",
Implementation tip: Py_BuildValue() and PyObject_CallMethod() accepts NULL to be
passed for the 'O' format code: they quickly return NULL. And the 'N' format
avoids the need to DECREF.

http://codereview.appspot.com/1874048/diff/1/34
File Python/errors.c (right):

http://codereview.appspot.com/1874048/diff/1/34#newcode815
Python/errors.c:815: tmp = PyErr_ProgramText(filename, lineno);
PyErr_ProgramText() calls fopen() which won't work if filename is not decodable
with mbcs. Another function (PyErr_ProgramTextUnicode?) is needed.

http://codereview.appspot.com/1874048/diff/1/38
File Python/pythonrun.c (right):

http://codereview.appspot.com/1874048/diff/1/38#newcode703
Python/pythonrun.c:703: reencode_filename(PyObject *file, const char
*new_encoding,
Could you add a comment and explain why reencoding is necessary?

[haypo, 2010-07-31 21:40:52]

Answer to Amaury's comments.

http://codereview.appspot.com/1874048/diff/1/17
File Lib/test/test_xml_etree.py (right):

http://codereview.appspot.com/1874048/diff/1/17#newcode27
Lib/test/test_xml_etree.py:27: # ignore all tests if the current working
directory is undecodable
On 2010/07/31 11:33:01, amaury wrote:
> s/undecodable/unencodable/

fixed in r83363

http://codereview.appspot.com/1874048/diff/1/19
File Modules/main.c (right):

http://codereview.appspot.com/1874048/diff/1/19#newcode659
Modules/main.c:659: filename_bytes = PyUnicode_EncodeFSDefault(filenameObj);
On 2010/07/31 11:33:01, amaury wrote:
> Can PyUnicode_EncodeFSDefault() fail? In this case, the error should be
handled

Sure, especially on Windows because Python3 uses mbcs+strict on Windows (which
only support few characters). => fixed in r83364

http://codereview.appspot.com/1874048/diff/1/19#newcode663
Modules/main.c:663: p_cfilename = "<decoding error>";
On 2010/07/31 11:33:01, amaury wrote:
> Here we have an exception set, should PyErr_Clear() be called?

That was the previous behaviour. I don't know if it's really critical that
__file__ is wrong.

> Is there a way to test this code path?

Test on Windows with a filename containing a character not encodable to mbcs
(eg. Ł with code page 1252).

./python testŁ.py

testŁ.py: print(__file__)

http://codereview.appspot.com/1874048/diff/1/27
File PC/import_nt.c (right):

http://codereview.appspot.com/1874048/diff/1/27#newcode18
PC/import_nt.c:18: FILE *PyWin_FindRegisteredModule(const char *moduleName,
On 2010/07/31 11:33:01, amaury wrote:
> can module names be any unicode string? which encoding is used here?

I don't know. Is __import__('testé') a valid call?

http://codereview.appspot.com/1874048/diff/1/27#newcode22
PC/import_nt.c:22: /* FIXME: use wchar_t* type */
On 2010/07/31 11:33:01, amaury wrote:
> this function should use wchar_t everywhere, RegQueryValueW &co...

Before yesterday, I didn't tried my patch on Windows. And I don't have high
skills on Windows API. But yes, it would be better to use wchar_t*. That's why I
added a FIXME :-)

Since moduleName should(?) be ASCII only, I suppose that it can be fixed later.

http://codereview.appspot.com/1874048/diff/1/28
File Parser/tokenizer.c (right):

http://codereview.appspot.com/1874048/diff/1/28#newcode474
Parser/tokenizer.c:474: stream = PyObject_CallMethod(io, "open", "Osis",
On 2010/07/31 11:33:01, amaury wrote:
> Implementation tip: Py_BuildValue() and PyObject_CallMethod() accepts NULL to
be
> passed for the 'O' format code: they quickly return NULL. And the 'N' format
> avoids the need to DECREF.

Ok for N: done in r83367.

But I don't like the magical behaviour when NULL is passed to
PyObject_CallMethod() :-p

http://codereview.appspot.com/1874048/diff/1/34
File Python/errors.c (right):

http://codereview.appspot.com/1874048/diff/1/34#newcode815
Python/errors.c:815: tmp = PyErr_ProgramText(filename, lineno);
On 2010/07/31 11:33:01, amaury wrote:
> PyErr_ProgramText() calls fopen() which won't work if filename is not
decodable
> with mbcs. Another function (PyErr_ProgramTextUnicode?) is needed.

I realized today that my patch doesn't solve the whole issue (full unicode
support). There are still "some" functions using PyUnicode_DecodeFSDefault() /
PyUnicode_EncodeFSDefault(). Until all of these functions are fixed, Python will
not be fully unicode compliant on Windows.

But there are some critical functions like Parser/parsetok.c. I think that it
would be better to fix that later in another patch.

This patch is not enough but improve/fix many cases, even on Windows.

http://codereview.appspot.com/1874048/diff/1/38
File Python/pythonrun.c (right):

http://codereview.appspot.com/1874048/diff/1/38#newcode703
Python/pythonrun.c:703: reencode_filename(PyObject *file, const char
*new_encoding,
On 2010/07/31 11:33:01, amaury wrote:
> Could you add a comment and explain why reencoding is necessary?

In reencode_filename() or in initfsencoding()?

[merwok, 2010-07-31 22:15:40]

http://codereview.appspot.com/1874048/diff/1/9
File Lib/distutils/file_util.py (right):

http://codereview.appspot.com/1874048/diff/1/9#newcode228
Lib/distutils/file_util.py:228: f = open(filename, "w", encoding="utf-8",
errors="surrogateescape")
distutils is feature-frozen, and this is arguably a new feature, so I don’t know
if it will fly.

http://codereview.appspot.com/1874048/diff/1/13
File Lib/test/test_subprocess.py (right):

http://codereview.appspot.com/1874048/diff/1/13#newcode839
Lib/test/test_subprocess.py:839: self.assertEquals(exitcode, 0)
Opened #9443 to address that.

[merwok, 2010-08-10 21:40:30]

http://codereview.appspot.com/1874048/diff/1/9
File Lib/distutils/file_util.py (right):

http://codereview.appspot.com/1874048/diff/1/9#newcode228
Lib/distutils/file_util.py:228: f = open(filename, "w", encoding="utf-8",
errors="surrogateescape")
On 2010/07/30 13:35:47, Antoine Pitrou wrote:
> Why the hardcoding to utf-8?

Distutils writes files in UTF-8; it’s undocumented I think. See #9561 opened by
Haypo today.