Message87098
Reviewers: report_bugs.python.org, Benjamin,
Message:
Fixed in r72272
http://codereview.appspot.com/52095/diff/1/5
File Include/unicodeobject.h (right):
http://codereview.appspot.com/52095/diff/1/5#newcode1254
Line 1254: The function is intended to be used for paths and file names
only
On 2009/05/03 21:34:26, Benjamin wrote:
> If these are only meant to be used internally during bootstrap, should
they have
> the _Py prefix?
Perhaps. However, I only moved the declarations from a different part of
the file, so I feel renaming them is out of scope for this patch. Only
PyUnicode_FSConverter is new.
http://codereview.appspot.com/52095/diff/1/10
File Lib/test/test_pep383.py (right):
http://codereview.appspot.com/52095/diff/1/10#newcode1
Line 1: from test import support
On 2009/05/03 21:34:26, Benjamin wrote:
> I'm not sure this deserves a whole new file. Couldn't UtfbTest go in
test_codecs
> and FileTests go in test_os?
Done.
http://codereview.appspot.com/52095/diff/1/10#newcode2
Line 2: import unittest, shutil, os, sys
On 2009/05/03 21:34:26, Benjamin wrote:
> PEP 8 says these should be on separate lines.
Done.
http://codereview.appspot.com/52095/diff/1/10#newcode33
Line 33: if os.name != 'win32':
On 2009/05/03 21:34:26, Benjamin wrote:
> Isn't os.name "nt" on Windows?
class with
> @unittest.skipIf(sys.platform.startswith("win")).
Done. It's now in a non-Win32 section of test_os.
http://codereview.appspot.com/52095/diff/1/10#newcode47
Line 47: f = open(self.bdir + b"/" + fn, "w")
On 2009/05/03 21:34:26, Benjamin wrote:
> Looks like a good place for os.path.join.
Done.
http://codereview.appspot.com/52095/diff/1/13
File Modules/posixmodule.c (right):
http://codereview.appspot.com/52095/diff/1/13#newcode547
Line 547: else if(PyByteArray_Check(o)) {
On 2009/05/03 21:34:26, Benjamin wrote:
> There's a small white space problem here.
Sorry, I don't see it: what is the problem?
http://codereview.appspot.com/52095/diff/1/13#newcode548
Line 548: if (lock && o->ob_type->tp_as_buffer->bf_getbuffer(o, NULL, 0)
< 0)
On 2009/05/03 21:34:26, Benjamin wrote:
> I believe you want PyObject_GetBuffer here.
Done. Notice, however, the loss of symmetry with release_bytes, where an
equivalent API function does not appear to exist.
http://codereview.appspot.com/52095/diff/1/13#newcode596
Line 596: bytes2str(name, 0));
On 2009/05/03 21:34:26, Benjamin wrote:
> What if byte2str returns NULL?
It really shouldn't; bytes2str should only be applied to results of the
PyUnicode_FSConverter, which should have checked that the result is
either bytes or bytearray. I have now changed bytes2str to give a
FatalError otherwise.
http://codereview.appspot.com/52095/diff/1/2
File Python/codecs.c (right):
http://codereview.appspot.com/52095/diff/1/2#newcode852
Line 852: if (!res)
On 2009/05/03 21:34:26, Benjamin wrote:
> This leaks "object".
Done. Again :-(
http://codereview.appspot.com/52095/diff/1/2#newcode895
Line 895: return NULL;
On 2009/05/03 21:34:26, Benjamin wrote:
> This leaks "object".
Done.
Please review this at http://codereview.appspot.com/52095
Affected files:
M Doc/library/codecs.rst
M Doc/library/os.rst
M Include/unicodeobject.h
A Lib/test/test_pep383.py
M Modules/_io/fileio.c
M Modules/posixmodule.c
M Modules/python.c
M Objects/unicodeobject.c
M Python/codecs.c
M Python/pythonrun.c
M configure
M configure.in
M pyconfig.h.in |
|
Date |
User |
Action |
Args |
2009-05-04 05:01:12 | loewis | set | recipients:
+ loewis, benjamin.peterson |
2009-05-04 05:01:07 | loewis | link | issue5915 messages |
2009-05-04 05:00:40 | loewis | create | |
|