cpython (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 &&

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -1767,6 +1778,10 @@ _PyDict_Pop_KnownHash(PyObject dict, PyObject key, Py_hash_t hash, PyObject *d
Py_DECREF(old_key);

assert(_PyDict_CheckConsistency(mp));
if (after_removed(mp) < 0) {
Py_DECREF(old_value);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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: I have made the requested changes; please review again.

This is a 3.8 thing, right?