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

#24726: OrderedDict has strange behaviour when dict.__setitem__ is used.

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years, 5 months ago by mark
Modified:
4 years, 3 months ago
Reviewers:
ericsnowcurrently, storchaka
CC:
rhettinger, terry.reedy, Mark.Shannon, devnull_psf.upfronthosting.co.za, eric.snow, storchaka, mpaolini
Visibility:
Public.

Patch Set 1 #

Total comments: 4

Patch Set 2 #

Unified diffs Side-by-side diffs Delta from patch set Stats Patch
Lib/test/test_ordered_dict.py View 1 2 chunks +40 lines, -1 line 0 comments Download
Objects/odictobject.c View 1 3 chunks +7 lines, -2 lines 0 comments Download

Messages

Total messages: 2
eric.snow
Other than a couple minor comments, LGTM. http://bugs.python.org/review/24726/diff/15803/Lib/test/test_collections.py File Lib/test/test_collections.py (right): http://bugs.python.org/review/24726/diff/15803/Lib/test/test_collections.py#newcode2036 Lib/test/test_collections.py:2036: def test_dict_setitem(self): ...
4 years, 4 months ago #1
storchaka_gmail.com
4 years, 4 months ago #2
http://bugs.python.org/review/24726/diff/15803/Lib/test/test_collections.py
File Lib/test/test_collections.py (right):

http://bugs.python.org/review/24726/diff/15803/Lib/test/test_collections.py#n...
Lib/test/test_collections.py:2036: def test_dict_setitem(self):
On 2015/11/04 17:44:00, eric.snow wrote:
> Though __delitem__ and __setitem__ should cover the case pretty well, it may
> make sense to add test methods for the following other dict methods:
> 
>   clear()
>   pop()
>   popitem()
>   setdefault()
>   update()

Done.

http://bugs.python.org/review/24726/diff/15803/Objects/odictobject.c
File Objects/odictobject.c (right):

http://bugs.python.org/review/24726/diff/15803/Objects/odictobject.c#newcode1478
Objects/odictobject.c:1478: pieces = PyList_New(PyODict_SIZE(self));
On 2015/11/04 17:44:00, eric.snow wrote:
> An alternative would be to change the definition of PyODict_SIZE to be
> independent of the underlying dict.  It may or may not be worth it though.

This may be a separate issue. PyODict_SIZE is a macro and changing it in 3.5 can
be problematic.

Actually the patch uses PyODict_SIZE only as a hint for fast preallocating a
list in common case. Resulting list can be shorter or longer than PyODict_SIZE.
Sign in to reply to this message.

RSS Feeds Recent Issues | This issue
This is Rietveld 894c83f36cb7+