Implement portable array pickling. - Code Review (original) (raw)

Issue 87072: Implement portable array pickling. (Closed)

Can't Edit Can't Publish+Mail Start Review Created: 15 years, 10 months ago by Alexandre Vassalotti Modified: 15 years, 2 months ago Reviewers: Martin v. Löwis Base URL: http://svn.python.org/view/\*checkout\*/python/trunk/ Visibility: Public.	Patch Set 1# Total comments: 10 Patch Set 2 : Implement portable array pickling for Python 3.# Patch Set 3 : Patch with minor stylistic issues fixed.# Created: 15 years, 10 months ago Download[raw] [tar.bz2] Unified diffs Side-by-side diffs Delta from patch set Stats (+655 lines, -59 lines) Patch M Lib/test/test_array.py View 1 4 chunks +140 lines, -10 lines 0 comments Download M Modules/arraymodule.c View 1 2 6 chunks +515 lines, -49 lines 0 comments Download Messages Total messages: 2 Expand All Messages | Collapse All Messages Martin v. Löwis http://codereview.appspot.com/87072/diff/1/3 File Modules/arraymodule.c (left): http://codereview.appspot.com/87072/diff/1/3#oldcode629 Line 629: array_copy(arrayobject *a, PyObject *unused) Why did you remove ... 15 years, 10 months ago (2009-07-06 04:54:53 UTC)#1 http://codereview.appspot.com/87072/diff/1/3 File Modules/arraymodule.c (left): http://codereview.appspot.com/87072/diff/1/3#oldcode629 Line 629: array_copy(arrayobject *a, PyObject *unused) Why did you remove these parameters? It's important that they stay, and their removal seems unrelated to this patch, anyway. http://codereview.appspot.com/87072/diff/1/3 File Modules/arraymodule.c (right): http://codereview.appspot.com/87072/diff/1/3#newcode1059 Line 1059: bogus white-space only change. http://codereview.appspot.com/87072/diff/1/3#newcode1504 Line 1504: UNKNOWN_FORMAT = -1, Is it really necessary to support this UNKNOWN_FORMAT? I propose to drop it. If it is necessary, a comment should explain the list of cases under which it may get used. http://codereview.appspot.com/87072/diff/1/3#newcode1654 Line 1654: static PyObject * It would be good if a comment explained what this function is supposed to do. http://codereview.appspot.com/87072/diff/1/3#newcode1664 Line 1664: typecode_obj = PyString_FromStringAndSize((char *)&typecode, 1); That sounds like a typing error: you shouldn't reinterpret an int as a char[]. Are you sure this code actually works on big-endian machines, where &typecode points to the MSB? http://codereview.appspot.com/87072/diff/1/3#newcode1760 Line 1760: return NULL; This leaks converted_items. http://codereview.appspot.com/87072/diff/1/3#newcode1806 Line 1806: case UNSIGNED_INT8: I think this integer conversion is incorrect (see msg71050). When unpickling an l-code array on a 64-bit machine that was pickled on a 32-bit machine, I think it should unpickle as an i-code array instead. http://codereview.appspot.com/87072/diff/1/3#newcode1822 Line 1822: Py_ssize_t itemcount = Py_SIZE(items) / width; This should check for truncation, IMO. http://codereview.appspot.com/87072/diff/1/3#newcode1823 Line 1823: const unsigned char *memstr = \ The backslash isn't necessary here. http://codereview.appspot.com/87072/diff/1/3#newcode1888 Line 1888: /* We got something wierd; so convert the array to a list weird. Sign in to reply to this message. Alexandre Vassalotti On 2009/07/06 04:54:53, Martin v. Löwis wrote: > http://codereview.appspot.com/87072/diff/1/3#newcode1822 > Line 1822: Py_ssize_t itemcount = ... 15 years, 10 months ago (2009-07-06 23:12:31 UTC)#2 On 2009/07/06 04:54:53, Martin v. Löwis wrote: > http://codereview.appspot.com/87072/diff/1/3#newcode1822 > Line 1822: Py_ssize_t itemcount = Py_SIZE(items) / width; > This should check for truncation, IMO. > How can truncation occur? That's the only thing I didn't fix in my new patch set. Sign in to reply to this message. Expand All Messages	Collapse All Messages

Issue 87072: Implement portable array pickling. (Closed)
Created 15 years, 10 months ago by Alexandre Vassalotti
Modified 15 years, 2 months ago
Reviewers: Martin v. Löwis
Base URL: http://svn.python.org/view/\*checkout\*/python/trunk/
Comments: 10