Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(10)

Issue 3863042: Implement PEP 3118 struct changes

Can't Edit
Can't Publish+Mail
Start Review
Created:
8 years, 8 months ago by minge
Modified:
5 years 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
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
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 ...
8 years, 8 months ago (2011-01-07 03:59:45 UTC) #1
pv
8 years, 7 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.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b