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

#27350: Compact and ordered dict

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 6 months ago by songofacandy
Modified:
1 year, 3 months ago
Reviewers:
mark, jimjjewett
CC:
rhettinger, jcea, haypo, jab, inada.naoki, stoneleaf, devnull_psf.upfronthosting.co.za, storchaka, stefan_drees.name
Visibility:
Public.

Patch Set 1 #

Total comments: 13

Patch Set 2 #

Total comments: 2

Patch Set 3 #

Total comments: 39

Patch Set 4 #

Total comments: 8

Patch Set 5 #

Patch Set 6 #

Unified diffs Side-by-side diffs Delta from patch set Stats Patch
Include/object.h View 1 2 3 4 5 1 chunk +0 lines, -1 line 0 comments Download
Lib/test/test_descr.py View 1 2 3 4 5 1 chunk +4 lines, -2 lines 0 comments Download
Lib/test/test_ordered_dict.py View 1 2 3 4 5 3 chunks +27 lines, -6 lines 0 comments Download
Lib/test/test_sys.py View 1 2 3 4 5 2 chunks +5 lines, -5 lines 0 comments Download
Lib/test/test_weakref.py View 1 2 3 4 5 1 chunk +5 lines, -2 lines 0 comments Download
Misc/NEWS View 1 2 3 4 5 1 chunk +4 lines, -0 lines 0 comments Download
Objects/dict-common.h View 1 2 3 4 5 1 chunk +13 lines, -3 lines 0 comments Download
Objects/dictobject.c View 1 2 3 4 5 83 chunks +734 lines, -553 lines 0 comments Download
Objects/object.c View 1 2 3 4 5 1 chunk +0 lines, -6 lines 0 comments Download
Objects/odictobject.c View 1 2 3 4 5 2 chunks +8 lines, -5 lines 0 comments Download

Messages

Total messages: 10
Mark.Shannon
http://bugs.python.org/review/27350/diff/17682/Objects/dictobject.c File Objects/dictobject.c (right): http://bugs.python.org/review/27350/diff/17682/Objects/dictobject.c#newcode24 Objects/dictobject.c:24: | indcies | Typo http://bugs.python.org/review/27350/diff/17682/Objects/dictobject.c#newcode331 Objects/dictobject.c:331: * NOTE: Since ...
1 year, 5 months ago #1
inada.naoki
http://bugs.python.org/review/27350/diff/17682/Objects/dictobject.c File Objects/dictobject.c (right): http://bugs.python.org/review/27350/diff/17682/Objects/dictobject.c#newcode24 Objects/dictobject.c:24: | indcies | On 2016/06/19 21:49:24, Mark.Shannon wrote: > ...
1 year, 5 months ago #2
inada.naoki
http://bugs.python.org/review/27350/diff/17682/Objects/dictobject.c File Objects/dictobject.c (right): http://bugs.python.org/review/27350/diff/17682/Objects/dictobject.c#newcode334 Objects/dictobject.c:334: * If you tring n/2, you should increase GROWTH_RATE ...
1 year, 5 months ago #3
inada.naoki
http://bugs.python.org/review/27350/diff/17714/Objects/dict-common.h File Objects/dict-common.h (right): http://bugs.python.org/review/27350/diff/17714/Objects/dict-common.h#newcode28 Objects/dict-common.h:28: Py_ssize_t dk_nentries; /* How many entries is used. */ ...
1 year, 5 months ago #4
Jim.J.Jewett
I only skimmed the main one, dictobject.c ... but the smaller ones mostly look good, ...
1 year, 4 months ago #5
inada.naoki
http://bugs.python.org/review/27350/diff/17751/Include/object.h File Include/object.h (left): http://bugs.python.org/review/27350/diff/17751/Include/object.h#oldcode711 Include/object.h:711: PyAPI_FUNC(PyObject *) _PyDict_Dummy(void); On 2016/08/11 21:26:15, Jim.J.Jewett wrote: > ...
1 year, 4 months ago #6
inada.naoki
http://bugs.python.org/review/27350/diff/17751/configure File configure (right): http://bugs.python.org/review/27350/diff/17751/configure#newcode8149 configure:8149: On 2016/08/11 21:26:15, Jim.J.Jewett wrote: > Why no equivalent ...
1 year, 4 months ago #7
Jim.J.Jewett
https://bugs.python.org/review/27350/diff/18206/Objects/dictobject.c File Objects/dictobject.c (right): https://bugs.python.org/review/27350/diff/18206/Objects/dictobject.c#newcode16 Objects/dictobject.c:16: layout: Is this the layout for just dk_keys, as ...
1 year, 3 months ago #8
Jim.J.Jewett
https://bugs.python.org/review/27350/diff/18206/Objects/dict-common.h File Objects/dict-common.h (right): https://bugs.python.org/review/27350/diff/18206/Objects/dict-common.h#newcode16 Objects/dict-common.h:16: Py_ssize_t *hashpos); (I can't seem to reply to Naoki's ...
1 year, 3 months ago #9
inada.naoki
1 year, 3 months ago #10
https://bugs.python.org/review/27350/diff/18206/Objects/dict-common.h
File Objects/dict-common.h (right):

https://bugs.python.org/review/27350/diff/18206/Objects/dict-common.h#newcode16
Objects/dict-common.h:16: Py_ssize_t *hashpos);
On 2016/08/29 21:51:30, Jim.J.Jewett wrote:
> (I can't seem to reply to Naoki's comment directly.)
> Even if only two files in CPython itself include this file, it still looks
like
> a public header to me... and someone *could* have customized dicts by
supplying
> their own lookup function.  Not as easy as replacing the function for a
missing
> key, but still plausible ... which is why I think changing the type of
> dict_lookup_function should get a mention in whichever of what's new or
release
> notes is most comprehensive.

This header file is not included in Python installation. "make install" doesn't
put
this file to target directory. So I don't think this is public.

For example, PEP 412 changed this signature, but Python 3.3 what's new didn't
mention about this function.

I don't want to conflict with you, but I don't want to make release note too
noisy
by small internal design changes.
I'll add it when more one +1 from core devs to adding this function signature
change to NEWS.

https://bugs.python.org/review/27350/diff/18206/Objects/dictobject.c
File Objects/dictobject.c (right):

https://bugs.python.org/review/27350/diff/18206/Objects/dictobject.c#newcode16
Objects/dictobject.c:16: layout:
On 2016/08/29 21:16:07, Jim.J.Jewett wrote:
> Is this the layout for just dk_keys, as opposed to the whole dict?  I don't
see
> where ma_values appears, even though you refer to it in the wording below.

ma_values is only used for split-table.
In combined table, PyDictKeysObject has whole keys and values.
ma_values is not shown in this section because it is not a member of
PyDictKeyObject.

https://bugs.python.org/review/27350/diff/18206/Objects/dictobject.c#newcode57
Objects/dictobject.c:57: Only string (unicode) keys are allowed.
On 2016/08/29 21:16:07, Jim.J.Jewett wrote:
> , and the insertion order matches for all dict instances using the same keys
> instance. 

Done.

https://bugs.python.org/review/27350/diff/18206/Objects/dictobject.c#newcode1003
Objects/dictobject.c:1003: /* When insertion order is different from shared key,
combine it */
On 2016/08/29 21:16:07, Jim.J.Jewett wrote:
> I wonder if this (and similar wording) would be easier to understand if
"combine
> it" were changed to "split this instance into a separate dictionary (of
combined
> type)"

I don't like "split this instance" means "stop split table".  The word "split"
is used
for two reverse meanings.
So I changed it to "Convert this instance to combined table."
Sign in to reply to this message.

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