Martin v. Löwis <mar...@v.loewis.de> added the comment: 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 ---------- _______________________________________ Python tracker <rep...@bugs.python.org> <http://bugs.python.org/issue5915> _______________________________________ _______________________________________________ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
