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

Issue 52081: [issue3672] Ill-formed surrogates not treated as errors during encoding/decoding (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years ago by Martin v. Löwis
Modified:
9 years, 3 months ago
Reviewers:
Benjamin, report
Base URL:
http://svn.python.org/view/*checkout*/python/branches/pep-0383/
Visibility:
Public.

Patch Set 1 #

Total comments: 16

Patch Set 2 : Addressed first round of comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+200 lines, -21 lines) Patch
M Doc/library/codecs.rst View 1 1 chunk +12 lines, -0 lines 0 comments Download
M Lib/test/test_bytes.py View 1 1 chunk +2 lines, -2 lines 0 comments Download
M Lib/test/test_codecs.py View 1 2 chunks +13 lines, -2 lines 0 comments Download
M Lib/test/test_unicode.py View 1 1 chunk +3 lines, -3 lines 0 comments Download
M Lib/test/test_unicodedata.py View 1 2 chunks +2 lines, -1 line 0 comments Download
M Objects/unicodeobject.c View 1 11 chunks +72 lines, -11 lines 0 comments Download
M Python/codecs.c View 1 3 chunks +92 lines, -0 lines 0 comments Download
M Python/marshal.c View 1 2 chunks +4 lines, -2 lines 0 comments Download

Messages

Total messages: 2
Benjamin
http://codereview.appspot.com/52081/diff/1/5 File Doc/library/codecs.rst (right): http://codereview.appspot.com/52081/diff/1/5#newcode326 Line 326: In addition, the following error handlers are specific ...
10 years ago (2009-05-01 21:25:44 UTC) #1
Martin v. Löwis
10 years ago (2009-05-02 09:44:03 UTC) #2
Issues fixed in r72188.

http://codereview.appspot.com/52081/diff/1/5
File Doc/library/codecs.rst (right):

http://codereview.appspot.com/52081/diff/1/5#newcode326
Line 326: In addition, the following error handlers are specific to only
selected
On 2009/05/01 21:25:44, Benjamin wrote:
> "In addition, the following error handlers are specific to a single codec."
> sounds better

Done.

http://codereview.appspot.com/52081/diff/1/5#newcode335
Line 335: 
On 2009/05/01 21:25:44, Benjamin wrote:
> There should probably be a versionchanged directive indicating that
"surrogates"
> was added in 3.1.

Done.

http://codereview.appspot.com/52081/diff/1/6
File Lib/test/test_codecs.py (right):

http://codereview.appspot.com/52081/diff/1/6#newcode544
Line 544: def test_surrogates(self):
On 2009/05/01 21:25:44, Benjamin wrote:
> I think this should be split into 2 tests. "test_lone_surrogates" and
> "test_surrogate_handler".

Done.

http://codereview.appspot.com/52081/diff/1/4
File Objects/unicodeobject.c (right):

http://codereview.appspot.com/52081/diff/1/4#newcode157
Line 157: static PyObject *unicode_encode_call_errorhandler(const char *errors,
On 2009/05/01 21:25:44, Benjamin wrote:
> These prototypes are longer than 80 chars some places. I don't think the
> arguments need to line up with the starting parenthesis.

Done.

http://codereview.appspot.com/52081/diff/1/4#newcode2393
Line 2393: s, size, &exc, i-1, i, &newpos);
On 2009/05/01 21:25:44, Benjamin wrote:
> "exc" is never Py_DECREFed.

Done.

http://codereview.appspot.com/52081/diff/1/4#newcode4110
Line 4110: if (!PyUnicode_Check(repunicode)) {
On 2009/05/01 21:25:44, Benjamin wrote:
> Is there a test of this case somewhere?

No. This is temporary - for PEP 383, I will have to support error handlers
returning bytes here, also.

http://codereview.appspot.com/52081/diff/1/2
File Python/codecs.c (right):

http://codereview.appspot.com/52081/diff/1/2#newcode758
Line 758: if (PyObject_IsInstance(exc, PyExc_UnicodeEncodeError)) {
On 2009/05/01 21:25:44, Benjamin wrote:
> I believe PyErr_GivenExceptionMatches is more appropriate here, but given the
> rest of the file uses PyObject_IsInstance, it likely doesn't matter.

No. The interface is that an exception instance must be passed;
GivenExceptionMatches would also allow for tuples and types.

http://codereview.appspot.com/52081/diff/1/2#newcode771
Line 771: return NULL;
On 2009/05/01 21:25:44, Benjamin wrote:
> This is leaks "object".

Done.
Sign in to reply to this message.

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