[issue25410] Clean up and fix OrderedDict

Raymond Hettinger added the comment: > Maybe it's considered undefined behavior for a subclass to use > a method of one of its bases which it has overriden. In general, it's true that if OrderedDict is a subclass of dict, then it would have no defense against someone making a direct call to

[issue25410] Clean up and fix OrderedDict

Dan Snider added the comment: It might be more appropriate to start a new issue for this, but I'll leave that decision to somehow who would know for sure. Anyway, basically the entire dict/PyDictObject api functions do not appear work at all with OrderedDict. Or rather, OrderedDict doesn't

[issue25410] Clean up and fix OrderedDict

Cheryl Sabella added the comment: Is this issue still relevant? -- nosy: +csabella ___ Python tracker ___

[issue25410] Clean up and fix OrderedDict

Serhiy Storchaka added the comment: Proposed patch makes OrderedDict.copy() more consistent between implementations. -- stage: needs patch -> patch review Added file: http://bugs.python.org/file41418/odict_copy_iter.patch ___ Python tracker

[issue25410] Clean up and fix OrderedDict

Serhiy Storchaka added the comment: And even simpler example: list(od.keys()) is [1, 3] in 3.4 and [0, 1, 2, 3, 4] in 3.5. -- ___ Python tracker ___

[issue25410] Clean up and fix OrderedDict

Serhiy Storchaka added the comment: Following code prints X([(1, 1), (3, 3)]) on 3.4 and X([(0, 0), (1, 1), (2, 2), (3, 3), (4, 4)]) on 3.5+. from collections import OrderedDict class X(OrderedDict): def __iter__(self): for k in OrderedDict.__iter__(self): if k % 2:

[issue25410] Clean up and fix OrderedDict

Roundup Robot added the comment: New changeset 45ce4c6b4f36 by Serhiy Storchaka in branch '3.5': Issue #25410: Made testing that od_fast_nodes and dk_entries are in sync more https://hg.python.org/cpython/rev/45ce4c6b4f36 New changeset c16af48153a4 by Serhiy Storchaka in branch 'default': Issue

[issue25410] Clean up and fix OrderedDict

Serhiy Storchaka added the comment: Thank you for your review Eric. I made error in commit messages, that is why they are non shown here. odict_revert_setting_on_error.patch and odict_iternext_simpler.patch were committed in 1594c23d8c2f and ad44d551c13c. od_resize_sentinel2 in

[issue25410] Clean up and fix OrderedDict

Eric Snow added the comment: All 3 patches look fine to me. In "odict_resize_sentinel.patch", it would be nice if you could accomplish that with a single sentinel. However, fixing the bug is more critical. -- ___ Python tracker

[issue25410] Clean up and fix OrderedDict

Changes by Eric Snow : -- stage: patch review -> commit review ___ Python tracker ___

[issue25410] Clean up and fix OrderedDict

Serhiy Storchaka added the comment: Could you please make a review of last three patches Eric? -- ___ Python tracker ___

[issue25410] Clean up and fix OrderedDict

Eric Snow added the comment: I will review those patches soon. -- ___ Python tracker ___ ___ Python-bugs-list

[issue25410] Clean up and fix OrderedDict

Serhiy Storchaka added the comment: Thanks Eric. A comment in PyODict_SetItem suggests to revert setting the value on the dict if adding new node failed. Following patch implements this suggestion. After committing the patch in issue25462 PyDict_DelItem can be replaced with

[issue25410] Clean up and fix OrderedDict

Serhiy Storchaka added the comment: Following patch makes the code for the iterating a little simpler by merging common code. -- Added file: http://bugs.python.org/file40840/odict_iternext_simpler.patch ___ Python tracker

[issue25410] Clean up and fix OrderedDict

Roundup Robot added the comment: New changeset a42c0c1c5133 by Serhiy Storchaka in branch '3.5': Issue #25410: C implementation of OrderedDict now uses type(self) instead of https://hg.python.org/cpython/rev/a42c0c1c5133 New changeset 10b965d59b49 by Serhiy Storchaka in branch 'default': Issue

[issue25410] Clean up and fix OrderedDict

Serhiy Storchaka added the comment: In very rare circumstances it is possible that after a series of modifications using raw dict methods, ma_keys becomes to point to the same address, but allocated keys array has different size. But the guard in _odict_get_index uses only the address as

[issue25410] Clean up and fix OrderedDict

Eric Snow added the comment: both patches* LGTM * odict_type.patch and odict_add_new_node_leak.patch -- ___ Python tracker ___

[issue25410] Clean up and fix OrderedDict

Eric Snow added the comment: Since the python-dev discussion about __class__, leaving the Python implementation alone is fine with me. -- stage: patch review -> commit review ___ Python tracker

[issue25410] Clean up and fix OrderedDict

Serhiy Storchaka added the comment: Thank you for your reviews and discussions (and for your appreciated C acceleration of OrderedDict of course) Eric. I just want to make the code a little cleaner and reliable. As for odict_type.patch, I would prefer to commit only C part of the patch and

[issue25410] Clean up and fix OrderedDict

Roundup Robot added the comment: New changeset 93f948120773 by Serhiy Storchaka in branch '3.5': Issue #25410: Fixed a memory leak in OrderedDict in the case when key's hash https://hg.python.org/cpython/rev/93f948120773 New changeset c3cec0f77eff by Serhiy Storchaka in branch 'default': Issue

[issue25410] Clean up and fix OrderedDict

Eric Snow added the comment: And thanks again, Serhiy, for taking the time on this. :) -- ___ Python tracker ___

[issue25410] Clean up and fix OrderedDict

Serhiy Storchaka added the comment: Seems there is a leak in _odict_add_new_node() when PyObject_Hash(key) fails. Here is a fix. -- stage: commit review -> patch review Added file: http://bugs.python.org/file40817/odict_add_new_node_leak.patch ___

[issue25410] Clean up and fix OrderedDict

Serhiy Storchaka added the comment: Here is a patch that makes both implementations to use type(self) instead of self.__class__ in __repr__(), __reduce__() and copy(). There is a difference between current implementations. Python implementation uses self.__class__ in copy(), C implementation

[issue25410] Clean up and fix OrderedDict

Roundup Robot added the comment: New changeset b6e33798f82a by Serhiy Storchaka in branch '3.5': Issue #25410: Cleaned up and fixed minor bugs in C implementation of OrderedDict. https://hg.python.org/cpython/rev/b6e33798f82a New changeset 741ef17e9b86 by Serhiy Storchaka in branch 'default':

[issue25410] Clean up and fix OrderedDict

Eric Snow added the comment: Posted to python-dev: https://mail.python.org/pipermail/python-dev/2015-October/141953.html https://mail.python.org/pipermail/python-dev/2015-October/141954.html -- stage: patch review -> commit review ___ Python

[issue25410] Clean up and fix OrderedDict

Serhiy Storchaka added the comment: Thank you for your review Eric. As for using Py_TYPE(self) instead of the __class__ attribute in #3, this is consistent with the rest of Python core and stdlib. All C implementations use Py_TYPE(self) for repr and pickling, even if Python implementations

[issue25410] Clean up and fix OrderedDict

Eric Snow added the comment: Regarding #7, I see what you did now. That looks fine to me. -- ___ Python tracker ___

[issue25410] Clean up and fix OrderedDict

Serhiy Storchaka added the comment: > Regarding Py_TYPE(od) vs. od.__class__, there is a difference for > subclasses, as you demonstrated in your example. [1] Thanks for explaining > your rationale. I now understand your argument about using PyTYPE() for > repr and pickle in C types. I still

[issue25410] Clean up and fix OrderedDict

Serhiy Storchaka added the comment: Updated patch addresses Eric's comments. Changes related to #3 are reverted. We will return to this after discussing on Python-Dev. -- ___ Python tracker

[issue25410] Clean up and fix OrderedDict

Changes by Serhiy Storchaka : Added file: http://bugs.python.org/file40803/odict_cleanup_2.patch ___ Python tracker ___

[issue25410] Clean up and fix OrderedDict

Eric Snow added the comment: Regarding Py_TYPE(od) vs. od.__class__, there is a difference for subclasses, as you demonstrated in your example. [1] Thanks for explaining your rationale. I now understand your argument about using PyTYPE() for repr and pickle in C types. I still have

[issue25410] Clean up and fix OrderedDict

Eric Snow added the comment: Regarding #5, you're right about OrderedDict().__dict__ being empty for the C implementation. (Nice observation!) So I'm okay with ripping all that code out of odict_reduce(). Since we're still accessing od.__dict__ through _PyObject_GetAttrId() that should not

[issue25410] Clean up and fix OrderedDict

Eric Snow added the comment: new patch LGTM -- ___ Python tracker ___ ___ Python-bugs-list mailing list

[issue25410] Clean up and fix OrderedDict

Eric Snow added the comment: > Backward compatibility related to __class__ assignment was already broken in C > implementation. In 3.4 following code works: [snip] > In 3.5 it doesn't. Depending on what feedback we get from python-dev, that may need to be fixed. I'd be inclined to not worry

[issue25410] Clean up and fix OrderedDict

Eric Snow added the comment: Thanks for working on this, Serhiy. I've left a review. As to the points you outlined, I have concerns with the impact of #3 and #5 on subclasses. Notably od.__class__ is not necessarily the same as type(od). Also #7 may introduce an unhandled re-entrancy,

[issue25410] Clean up and fix OrderedDict

New submission from Serhiy Storchaka: Proposed patch cleans up and fixes minor bugs in C implementation of OrderedDict. 1. Used the "p" format unit instead of manual calling PyObject_True() for parsing boolean parameters. 2. Used _Py_Identifier private API instead of char* API if

[issue25410] Clean up and fix OrderedDict

Raymond Hettinger added the comment: These all look good except for perhaps #5 which I need to look at a bit more for its effect on OD subclasses. -- ___ Python tracker