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

Issue 91044: Fix the last regression shown by cvs2svn (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 months, 1 week ago by Collin Winter
Modified:
3 months, 1 week ago
Reviewers:
Jeffrey Yasskin
CC:
unladen-swallow_googlegroups.com
SVN Base:
http://unladen-swallow.googlecode.com/svn/trunk/
Visibility:
Public.

Description

We need to hold a reference to the memoized object in the Pickler for as long as
the Pickler lives.

With this applied, all of cvs2svn's tests that pass under mainline 2.6.1 pass
under Unladen Swallow.

Patch Set 1

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats Patch
M Modules/_testcapimodule.c View 2 chunks 87 lines 1 comment Download
M Modules/cPickle.c View 12 chunks 121 lines 1 comment Download

Messages

Total messages: 2
Collin Winter
PTAL
4 months, 1 week ago
Jeffrey Yasskin
4 months, 1 week ago
LGTM

http://codereview.appspot.com/91044/diff/1/2
File Modules/_testcapimodule.c (right):

http://codereview.appspot.com/91044/diff/1/2#newcode834
Line 834: Py_DECREF(three);  /* The memo table takes its own reference. */
This comment seems unnecessary, since incref'ing on insertion is pretty common
in Python. On the other hand, you should have a test that checks that the
objects aren't deallocated while they're in the memotable.

http://codereview.appspot.com/91044/diff/1/3
File Modules/cPickle.c (right):

http://codereview.appspot.com/91044/diff/1/3#newcode382
Line 382: Py_ssize_t i;
Just call PyMemoTable_Clear()? The extra memset shouldn't cost that much since
everything was just decref'ed, and it might even be optimized away.
Sign in to reply to this message.

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