This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: Use PyDict_Contains() and PyDict_SetDefault() instead of PyDict_GetItemWithError()
Type: enhancement Stage: resolved
Components: Interpreter Core Versions: Python 3.10
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: methane, serhiy.storchaka, vstinner
Priority: normal Keywords: patch

Created on 2020-10-26 07:16 by serhiy.storchaka, last changed 2022-04-11 14:59 by admin. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 22986 merged serhiy.storchaka, 2020-10-26 07:41
Messages (5)
msg379647 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2020-10-26 07:39
It was common to use the code PyDict_GetItem(dict, key) == NULL to check whether the key is in the dict. PyDict_GetItem() returns borrowed reference, so no clean up is needed. And if we need to check only existence of the key, we do not need to store a value.

But PyDict_GetItem() which suppresses all internal exceptions is considered not safe, so PyDict_GetItemWithError() should be used. The code looks like:

    if (PyDict_GetItemWithError(dict, key) == NULL) {
        if (PyErr_Occurred()) {
            goto error;
        }
        // does not have the key
    }
    else {
        // has the key
    }

It can be written with using PyDict_Contains():

    int r = PyDict_Contains(dict, key);
    if (r < 0) {
        goto error;
    }
    if (r == 0) {
        // does not have the key
    }
    else {
        // has the key
    }

Advantages:

* It is more clear expression of what we do.
* It is more simple and efficient in PyPy, because it has to keep borrowed references and track their lifetime.
* It may be more efficient in CPython, because calling PyErr_Occurred() has small but non-zero cost.
* PyErr_Occurred() would not be fooled by exception raised before. So you can use this code even if an exception is set.

Disadvantages:

* You need to use an int variable.

In some cases, when this check is used in combinations with PyDict_SetItem(), the code can be replaced with PyDict_SetDefault(), which is bot more terse and efficient. And when it is used in combinations with PyDict_DelItem(), the code can be replaced with _PyDict_Pop().

The proposed patch makes the code using PyDict_Contains() and PyDict_SetDefault() if appropriate. All use cases for _PyDict_Pop() were already covered by previous changes.
msg379648 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2020-10-26 07:50
The code uses also _PyDict_ContainsId() added in issue42006 instead of _PyDict_GetItemIdWithError().

Few uses of _PyDict_GetItemStringWithError() are left as is. I do not think it is worth to introduce _PyDict_ContainsString(). They could be rewritten using _PyDict_ContainsId() if we find it worth.
msg379659 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2020-10-26 10:48
New changeset b510e101f8b5b31276bf97b921ca9247162881d2 by Serhiy Storchaka in branch 'master':
bpo-42152: Use PyDict_Contains and PyDict_SetDefault if appropriate. (GH-22986)
https://github.com/python/cpython/commit/b510e101f8b5b31276bf97b921ca9247162881d2
msg391033 - (view) Author: Inada Naoki (methane) * (Python committer) Date: 2021-04-14 05:14
May I close this issue?
msg391037 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2021-04-14 07:12
Sure. Thanks for reminder.
History
Date User Action Args
2022-04-11 14:59:37adminsetgithub: 86318
2021-04-14 07:12:00serhiy.storchakasetstatus: open -> closed
resolution: fixed
messages: + msg391037

stage: patch review -> resolved
2021-04-14 05:14:52methanesetmessages: + msg391033
2020-10-26 10:48:11serhiy.storchakasetmessages: + msg379659
2020-10-26 07:50:36serhiy.storchakasetmessages: + msg379648
2020-10-26 07:41:35serhiy.storchakasetkeywords: + patch
stage: patch review
pull_requests: + pull_request21900
2020-10-26 07:39:38serhiy.storchakasettype: enhancement
title: Use PyDict_Contains() and PyDict_SetDefault() instead of PyDict_GetItemIdWithError -> Use PyDict_Contains() and PyDict_SetDefault() instead of PyDict_GetItemWithError()
components: + Interpreter Core

nosy: + vstinner, methane
versions: + Python 3.10
messages: + msg379647
2020-10-26 07:16:24serhiy.storchakacreate