Implement PEP 3118 struct changes - Code Review (original) (raw)

Issue 3863042: Implement PEP 3118 struct changes

| Can't Edit Can't Publish+Mail Start Review Created: 14 years, 4 months ago by minge Modified: 10 years, 8 months ago Reviewers: pv, dickinsm Base URL: http://svn.python.org/view/\*checkout\*/python/branches/py3k/ Visibility: Public. | Description This patch implements the current work in a first step towards implementing the extended struct format string support called for in PEP 3118. Namely, the support for nested structures via 'T{}' is implemented. Patch Set 1# Total comments: 8 Created: 14 years, 4 months ago Download[raw] [tar.bz2] Unified diffs Side-by-side diffs Delta from patch set Stats (+842 lines, -243 lines) Patch Doc/library/struct.rst View 4 chunks +58 lines, -2 lines 2 comments Download Lib/test/test_struct.py View 2 chunks +185 lines, -0 lines 1 comment Download Modules/_struct.c View 17 chunks +599 lines, -241 lines 5 comments Download Messages Total messages: 2 Expand All Messages | Collapse All Messages minge http://codereview.appspot.com/3863042/diff/1/Modules/\_struct.c File Modules/_struct.c (right): http://codereview.appspot.com/3863042/diff/1/Modules/\_struct.c#newcode1666 Modules/_struct.c:1666: PyObject *tup = PyTuple_New(0); Since we know the number ... 14 years, 4 months ago (2011-01-07 03:59:45 UTC)#1 http://codereview.appspot.com/3863042/diff/1/Modules/_struct.c File Modules/_struct.c (right): http://codereview.appspot.com/3863042/diff/1/Modules/_struct.c#newcode1666 Modules/_struct.c:1666: PyObject *tup = PyTuple_New(0); Since we know the number of arguments that we are unpacking, we should use 'PyTuple_New(tree->s_len)' here followed by 'PyTuple_SetItem" calls in the loop. Then we can get rid off all the nasty 'PySequence_Concat' calls. Sign in to reply to this message. pv Some comments attached. I wrote some additional things you may wish to put in: (a) ... 14 years, 2 months ago (2011-02-12 19:31:43 UTC)#2 Some comments attached. I wrote some additional things you may wish to put in: (a) Support for the '^' byte order character: https://bitbucket.org/pv/cpython-stuff/changeset/ab9653885bd5 (b) Support for the 'i:fieldname:' field names. They are just skipped, though, but it's probably a good idea to try to support the PEP 3118 syntax as closely as feasible... https://bitbucket.org/pv/cpython-stuff/changeset/e04591f57ccb http://codereview.appspot.com/3863042/diff/1/Doc/library/struct.rst File Doc/library/struct.rst (right): http://codereview.appspot.com/3863042/diff/1/Doc/library/struct.rst#newcode99 Doc/library/struct.rst:99: The "^" code is missing from the table below. http://codereview.appspot.com/3863042/diff/1/Doc/library/struct.rst#newcode152 Doc/library/struct.rst:152: Also "^" does not add padding. http://codereview.appspot.com/3863042/diff/1/Lib/test/test_struct.py File Lib/test/test_struct.py (right): http://codereview.appspot.com/3863042/diff/1/Lib/test/test_struct.py#newcode697 Lib/test/test_struct.py:697: The tests for alignment handling is not fully complete. I wonder if the tests from Numpy's implementation could be ported here; although also that takes some trouble... http://codereview.appspot.com/3863042/diff/1/Modules/_struct.c File Modules/_struct.c (right): http://codereview.appspot.com/3863042/diff/1/Modules/_struct.c#newcode1301 Modules/_struct.c:1301: return isdigit(c); This probably shouldn't check for isdigit (count => it's not primitive). The count needs only be parsed by `parse_format_string_body`. http://codereview.appspot.com/3863042/diff/1/Modules/_struct.c#newcode1307 Modules/_struct.c:1307: static char *byte_order_codes = "<>!=@"; What about the '^' byte order marker? It's introduced in pep-3118 as native-endian no-alignment. http://codereview.appspot.com/3863042/diff/1/Modules/_struct.c#newcode1449 Modules/_struct.c:1449: while (*state->fmt == 'T' | | is_primitive(*state->fmt)) { ... | | isdigit(*state->fmt) http://codereview.appspot.com/3863042/diff/1/Modules/_struct.c#newcode1824 Modules/_struct.c:1824: n = tree->s_size; Should too long an input string be an error? Sign in to reply to this message. Expand All Messages | Collapse All Messages | | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | ------------------------------------ | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ----------------------------------------------------------------------------------------------- |

Issue 3863042: Implement PEP 3118 struct changes
Created 14 years, 4 months ago by minge
Modified 10 years, 8 months ago
Reviewers: dickinsm, pv
Base URL: http://svn.python.org/view/\*checkout\*/python/branches/py3k/
Comments: 8