New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
pickle.py's load_appends should call append() on objects other than lists #61920
Comments
In pickle.py, load_appends is currently defined as def load_appends(self):
stack = self.stack
mark = self.marker()
list = stack[mark - 1]
list.extend(stack[mark + 1:])
del stack[mark:] However, according to the spec of APPENDS, it should actually be: def load_appends(self):
obj = stack[mark - 1]
items = stack[mark + 1:]
if isinstance(obj, list):
obj.extend(items)
else:
for item in items:
obj.append(item) This will match with the current behaviour of _pickle. |
Your patch is bogus (duplicated load_appends definition). |
Yes, test is needed. Current tests don't cover this case. I suggest to move obj.append out of the tight loop, as in C implementation. append = obj.append
for item in items:
append(item) |
Alright alright! Here's a less bogus patch. :) |
New changeset 37139694aed0 by Alexandre Vassalotti in branch '3.3': |
Why this patch was not applied to 2.7? What is the spec of APPENDS? |
From PEP-7:
Both append() or extend() must be supported, therefore old code was correct. C implementation can be optimized by using extend(). |
I don't recall where I picked up the referred spec. Looking at PEP-307 and pickle docs 1, it does look like that objects which provides the listitems field of the reduce tuple should support extend() too. It might best to try both in the implementation to avoid breaking existing code that relies on the previous behavior. |
Thanks. Opened bpo-29368 for this. |
Sorry, I meant PEP-307 of course. |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: