[issue4136] merge json library with simplejson 2.0.3 - Code Review (original) (raw)

Can't Edit Can't Publish+Mail Start Review Created: 16 years, 6 months ago by bob.ippolito Modified: 15 years, 9 months ago Reviewers: Martin v. Löwis CC: report_bugs.python.org Base URL: http://svn.python.org/view/\*checkout\*/python/trunk/ Visibility: Public.	Patch Set 1# Downloaded from: http://bugs.python.org/file11822/json_issue4136_r66961.diff Created: 16 years, 6 months ago Download[raw] [tar.bz2] Unified diffs Side-by-side diffs Delta from patch set Stats (+1984 lines, -450 lines) Patch Lib/json/__init__.py View 5 chunks +16 lines, -15 lines 0 comments Download Lib/json/decoder.py View 10 chunks +93 lines, -102 lines 20 comments Download Lib/json/encoder.py View 10 chunks +202 lines, -150 lines 0 comments Download Lib/json/scanner.py View 1 chunk +55 lines, -58 lines 0 comments Download Lib/json/tests/test_decode.py View 1 chunk +7 lines, -0 lines 0 comments Download Lib/json/tests/test_float.py View 1 chunk +5 lines, -0 lines 0 comments Download Lib/json/tool.py View 1 chunk +0 lines, -1 line 0 comments Download Modules/_json.c View 19 chunks +1606 lines, -124 lines 12 comments Download Messages Total messages: 3 Expand All Messages | Collapse All Messages bob.ippolito 16 years, 6 months ago (2008-10-17 23:39:27 UTC)#1 Sign in to reply to this message. Martin v. Löwis http://codereview.appspot.com/7311/diff/1/8 File Lib/json/decoder.py (right): http://codereview.appspot.com/7311/diff/1/8#newcode55 Line 55: def py_scanstring(s, end, encoding=None, strict=True, _b=BACKSLASH, _m=STRINGCHUNK.match): This ... 16 years, 4 months ago (2009-01-04 13:22:28 UTC)#2 http://codereview.appspot.com/7311/diff/1/8 File Lib/json/decoder.py (right): http://codereview.appspot.com/7311/diff/1/8#newcode55 Line 55: def py_scanstring(s, end, encoding=None, strict=True, _b=BACKSLASH, _m=STRINGCHUNK.match): This function should get some comments what all the various cases are (preferably speaking with the terms of JSON spec, i.e. chars, char, ...) http://codereview.appspot.com/7311/diff/1/8#newcode71 Line 71: _append(content) # 3 cases: end of string, control character, escape sequence http://codereview.appspot.com/7311/diff/1/8#newcode76 Line 76: msg = "Invalid control character {0!r} at".format(esc) esc isn't assigned until a few lines later. Is this really correct? http://codereview.appspot.com/7311/diff/1/8#newcode104 Line 104: raise ValueError No message? http://codereview.appspot.com/7311/diff/1/8#newcode107 Line 107: raise ValueError No message? http://codereview.appspot.com/7311/diff/1/8#newcode111 Line 111: m = unichr(uni) What's the purpose of m? http://codereview.appspot.com/7311/diff/1/8#newcode127 Line 127: nextchar = s[end:end + 1] Why not s[end]? Add comment if this is necessary. http://codereview.appspot.com/7311/diff/1/8#newcode132 Line 132: nextchar = s[end:end + 1] Likewise. There are more places where it does slicing, but also places where it does indexing, in this function. http://codereview.appspot.com/7311/diff/1/8#newcode290 Line 290: following strings: -Infinity, Infinity, NaN. This sounds like an incompatible change. http://codereview.appspot.com/7311/diff/1/8#newcode317 Line 317: def raw_decode(self, s, idx=0): That looks like an incompatible change http://codereview.appspot.com/7311/diff/1/9 File Modules/_json.c (right): http://codereview.appspot.com/7311/diff/1/9#newcode196 Line 196: output_size *= 2; You might want to check for integer overflow here. http://codereview.appspot.com/7311/diff/1/9#newcode215 Line 215: ascii_escape_str(PyObject *pystr) Please attach a comment to each function, telling what the function does. http://codereview.appspot.com/7311/diff/1/9#newcode733 Line 733: "..." Some text should probably be added here. http://codereview.appspot.com/7311/diff/1/9#newcode1320 Line 1320: if ((idx + 3 < length) && str[idx + 1] == 'u' && str[idx + 2] == 'l' && str[idx + 3] == 'l') { Is this really faster than a strncmp? http://codereview.appspot.com/7311/diff/1/9#newcode1528 Line 1528: PyTypeObject PyScannerType = { I think scanner objects should participate in cyclic gc. http://codereview.appspot.com/7311/diff/1/9#newcode2025 Line 2025: "make_encoder", /* tp_name */ That is a confusing type name. How about "Encoder"? Sign in to reply to this message. bob.ippolito http://codereview.appspot.com/7311/diff/1/8 File Lib/json/decoder.py (right): http://codereview.appspot.com/7311/diff/1/8#newcode55 Line 55: def py_scanstring(s, end, encoding=None, strict=True, _b=BACKSLASH, _m=STRINGCHUNK.match): Commented ... 16 years, 4 months ago (2009-01-05 01:28:18 UTC)#3 http://codereview.appspot.com/7311/diff/1/8 File Lib/json/decoder.py (right): http://codereview.appspot.com/7311/diff/1/8#newcode55 Line 55: def py_scanstring(s, end, encoding=None, strict=True, _b=BACKSLASH, _m=STRINGCHUNK.match): Commented in the next patch. http://codereview.appspot.com/7311/diff/1/8#newcode71 Line 71: _append(content) Commented in the next patch http://codereview.appspot.com/7311/diff/1/8#newcode76 Line 76: msg = "Invalid control character {0!r} at".format(esc) This is a bug in the exception handling code, fixed in the next patch. http://codereview.appspot.com/7311/diff/1/8#newcode104 Line 104: raise ValueError Exception is caught at the except block and re-raised with a message. Next patch unrolls this so it's not confusing. http://codereview.appspot.com/7311/diff/1/8#newcode107 Line 107: raise ValueError Exception is caught at the except block and re-raised with a message. Next patch unrolls this so it's not confusing. http://codereview.appspot.com/7311/diff/1/8#newcode111 Line 111: m = unichr(uni) Renamed m to char in the next patch. http://codereview.appspot.com/7311/diff/1/8#newcode127 Line 127: nextchar = s[end:end + 1] commented in next patch (only once). s[end] can raise an IndexError with bad input, s[end:end+1] returns an empty string on underflow, which is caught later with a more helpful error message. http://codereview.appspot.com/7311/diff/1/8#newcode132 Line 132: nextchar = s[end:end + 1] commented in next patch (only once). s[end] can raise an IndexError with bad input, s[end:end+1] returns an empty string on underflow, which is caught later with a more helpful error message. http://codereview.appspot.com/7311/diff/1/8#newcode290 Line 290: following strings: -Infinity, Infinity, NaN. Not practically speaking. The documented purpose of this callback is "This can be used to raise an exception if invalid JSON numbers are encountered.". I've never seen it used to handle None, True, False in a different manner. That was more of an implementation detail than anything else, and that is fixed by this patch. Existing implementations of this callback will simply have dead code since they will never be called with "null", "true" or "false" anymore. http://codereview.appspot.com/7311/diff/1/8#newcode317 Line 317: def raw_decode(self, s, idx=0): It is a compatible change. http://codereview.appspot.com/7311/diff/1/9 File Modules/_json.c (right): http://codereview.appspot.com/7311/diff/1/9#newcode196 Line 196: output_size *= 2; _PyString_Resize checks for integer overflow, so it would explode there just fine. The next patch changes this slightly to avoid unnecessary calls to _PyString_Resize when the size didn't actually change, but doesn't do any explicit integer overflow checking http://codereview.appspot.com/7311/diff/1/9#newcode215 Line 215: ascii_escape_str(PyObject *pystr) Done in the next patch http://codereview.appspot.com/7311/diff/1/9#newcode733 Line 733: "..." Done in the next patch. http://codereview.appspot.com/7311/diff/1/9#newcode1320 Line 1320: if ((idx + 3 < length) && str[idx + 1] == 'u' && str[idx + 2] == 'l' && str[idx + 3] == 'l') { Probably not, but strncmp doesn't work for PyUnicode and the same code is repeated there. http://codereview.appspot.com/7311/diff/1/9#newcode1528 Line 1528: PyTypeObject PyScannerType = { I don't think it's possible to cause a cycle using the documented APIs, since the encoder is created and thrown away behind the scenes and never passed to user code. Someone else can write that patch if it's necessary. http://codereview.appspot.com/7311/diff/1/9#newcode2025 Line 2025: "make_encoder", /* tp_name */ It's not a type that's ever exposed to user code, make_encoder is somewhat less confusing because that's the name it's exposed as. I'll change it anyway though, it doesn't really matter since this is all private API. Sign in to reply to this message. Expand All Messages	Collapse All Messages

Issue 7311: [issue4136] merge json library with simplejson 2.0.3
Created 16 years, 6 months ago by bob.ippolito
Modified 15 years, 9 months ago
Reviewers: Martin v. Löwis
Base URL: http://svn.python.org/view/\*checkout\*/python/trunk/
Comments: 32