Issue 52095: [issue5915] PEP 383 implementation

Issue 52095: [issue5915] PEP 383 implementation (Closed)

Can't Edit
Can't Publish+Mail
Start Review

Created:
15 years, 10 months ago by Martin v. Löwis

Modified:
15 years, 1 month ago

Reviewers:
Benjamin, Antoine Pitrou, report

Base URL:
http://svn.python.org/view/*checkout*/python/branches/py3k/

Visibility:
Public.

Patch Set 2 : r72272 #

Total comments: 6

Created: 15 years, 10 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+717 lines, -288 lines)			Patch
M	Doc/library/codecs.rst	View		1 chunk	+2 lines, -0 lines	0 comments	Download
M	Doc/library/os.rst	View		3 chunks	+23 lines, -10 lines	0 comments	Download
M	Include/unicodeobject.h	View		4 chunks	+29 lines, -19 lines	0 comments	Download
M	Lib/test/test_codecs.py	View		2 chunks	+29 lines, -0 lines	2 comments	Download
M	Lib/test/test_os.py	View		3 chunks	+38 lines, -1 line	2 comments	Download
M	Modules/_io/fileio.c	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Modules/posixmodule.c	View	1	84 chunks	+314 lines, -205 lines	0 comments	Download
M	Modules/python.c	View		2 chunks	+89 lines, -24 lines	2 comments	Download
M	Objects/unicodeobject.c	View		4 chunks	+80 lines, -9 lines	0 comments	Download
M	Python/codecs.c	View	1	3 chunks	+89 lines, -0 lines	0 comments	Download
M	Python/pythonrun.c	View		2 chunks	+16 lines, -16 lines	0 comments	Download
M	configure	View		2 chunks	+3 lines, -2 lines	0 comments	Download
M	configure.in	View		1 chunk	+1 line, -1 line	0 comments	Download
M	pyconfig.h.in	View		1 chunk	+3 lines, -0 lines	0 comments	Download

Messages

Total messages: 4

Expand All Messages | Collapse All Messages

Benjamin

The calls to bytes2str are not checked in posixmodule.c. http://codereview.appspot.com/52095/diff/1/5 File Include/unicodeobject.h (right): http://codereview.appspot.com/52095/diff/1/5#newcode1254 Line ...

15 years, 10 months ago (2009-05-03 21:34:25 UTC) #1

Martin v. Löwis

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 ...

15 years, 10 months ago (2009-05-04 05:00:02 UTC) #2

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?
> 
> Also, you could skip this whole class on Windows by decorating the 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.

Antoine Pitrou

A couple of really small things. http://codereview.appspot.com/52095/diff/30/1013 File Lib/test/test_codecs.py (right): http://codereview.appspot.com/52095/diff/30/1013#newcode1545 Line 1545: b"foo\xa5bar") You ...

15 years, 10 months ago (2009-05-04 12:25:42 UTC) #3

Martin v. Löwis

15 years, 10 months ago (2009-05-05 04:02:24 UTC) #4

Fixed in r72310

http://codereview.appspot.com/52095/diff/30/1013
File Lib/test/test_codecs.py (right):

http://codereview.appspot.com/52095/diff/30/1013#newcode1545
Line 1545: b"foo\xa5bar")
On 2009/05/04 12:25:43, Antoine Pitrou wrote:
> You decode with iso-8859-3 but encode with iso-8859-4. Is it intended?

No, fixed.

http://codereview.appspot.com/52095/diff/30/1014
File Lib/test/test_os.py (right):

http://codereview.appspot.com/52095/diff/30/1014#newcode704
Line 704: filenames = [b'foo\xf6bar', b'foo\xf6bar']
On 2009/05/04 12:25:43, Antoine Pitrou wrote:
> Is my sight bad, or is this twice the same filename?
> 

Oops, fixed.

http://codereview.appspot.com/52095/diff/30/1016
File Modules/python.c (right):

http://codereview.appspot.com/52095/diff/30/1016#newcode47
Line 47: /* Try conversion with mbsrtwocs (C99), and escape non-decodable bytes.
*/
On 2009/05/04 12:25:43, Antoine Pitrou wrote:
> Typo: s/mbsrtwocs/mbrtowc/

Done.

Expand All Messages | Collapse All Messages