Repository navigation
Conversation
vstinner
left a comment
vstinner
left a comment
There was a problem hiding this comment.
I'm not sure of raising an exception if shrinking fails.
What do you think of being conservative: if shrinking fails, just ignore the error? The next del/pop will try again.
I'm not confortable with dict.pop() which is no longer atomic: an entry was removed and you cannot get it...
| static int | ||
| after_removed(PyDictObject *mp) | ||
| { | ||
| if (mp->ma_keys->dk_size > PyDict_MINSIZE && |
There was a problem hiding this comment.
Oh, I hesitated to propose the same test in your previous PR #5297, but I wasn't sure if it was required in practice since MINSIZE=8.
I like the idea of being explicit here, check size > MINSIZE.
|
|
||
| assert(_PyDict_CheckConsistency(mp)); | ||
| if (after_removed(mp) < 0) { | ||
| Py_DECREF(old_value); |
There was a problem hiding this comment.
I'm not sure that it's ok to drop the popped value here :-(
If shrinking the dict fails, I suggest to ignore the error and return the old value anyway. The next del/pop will try again to shrink the dict.
|
When you're done making the requested changes, leave the comment: |
|
This is a 3.8 thing, right? |
https://bugs.python.org/issue32623