Skip to content
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 and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add PyMapping_GetOptionalItem() #106307

Closed
serhiy-storchaka opened this issue Jul 1, 2023 · 3 comments
Closed

Add PyMapping_GetOptionalItem() #106307

serhiy-storchaka opened this issue Jul 1, 2023 · 3 comments
Labels
topic-C-API type-feature A feature request or enhancement

Comments

@serhiy-storchaka
Copy link
Member

serhiy-storchaka commented Jul 1, 2023

PyObject_GetItem() raises a KeyError if the key is not found in a mapping. In some cases it should be treated as any other error, but in other cases it should be caught and suppressed. The repeating pattern of PyObject_GetItem() followed by PyErr_ExceptionMatches(PyExc_KeyError) and PyErr_Clear() occurs 7 times in Python/bytecodes.c and at least 5 times in other files.

I propose to add private helper _PyMapping_LookupItem() which combines these three calls to make the code clearer. It also has a special case for exact dict, so eliminates even more repeating code. For example:

     PyObject *m;
-    if (PyDict_CheckExact(modules)) {
-        m = Py_XNewRef(PyDict_GetItemWithError(modules, name));
-    }
-    else {
-        m = PyObject_GetItem(modules, name);
-        if (_PyErr_ExceptionMatches(tstate, PyExc_KeyError)) {
-            _PyErr_Clear(tstate);
-        }
-    }
-    if (_PyErr_Occurred(tstate)) {
+    if (_PyMapping_LookupItem(modules, name, &m) < 0) {
         return NULL;
     }

The interface of _PyMapping_LookupItem() is very similar to other private helper _PyObject_LookupAttr() (see #76752) and to a proposed new C API for PyDict (see #106004).

Linked PRs

@serhiy-storchaka serhiy-storchaka added type-feature A feature request or enhancement topic-C-API labels Jul 1, 2023
serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this issue Jul 1, 2023
Replacement of PyObject_GetItem() which doesn't raise KeyError.

  int _PyMapping_LookupItem(PyObject *obj, PyObject *key, PyObject **result)

Return 1 and set *result != NULL if a key is found.
Return 0 and set *result == NULL if a key is not found;
a KeyError is silenced.
Return -1 and set *result == NULL if an error other than KeyError
is raised.
serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this issue Jul 1, 2023
Replacement of PyObject_GetItem() which doesn't raise KeyError.

  int _PyMapping_LookupItem(PyObject *obj, PyObject *key, PyObject **result)

Return 1 and set *result != NULL if a key is found.
Return 0 and set *result == NULL if a key is not found;
a KeyError is silenced.
Return -1 and set *result == NULL if an error other than KeyError
is raised.
@carljm
Copy link
Member

carljm commented Jul 3, 2023

Why should this be a private API? It seems like for many uses it's simply better, and it doesn't seem to expose any internal implementation details that we would be hesitant to support.

I think _PyObject_LookupAttr should probably be made public, in which case the leading underscore will be a wart that would be painful to remove but potentially confusing. Why not introduce PyMapping_LookupItem without the leading underscore in the first place?

@serhiy-storchaka
Copy link
Member Author

This is an interesting idea. If make _PyObject_LookupAttr public, then _PyMapping_LookupItem can be made public from the start.

@serhiy-storchaka serhiy-storchaka changed the title Add _PyMapping_LookupItem() Add PyMapping_GetOptionalItem() Jul 7, 2023
serhiy-storchaka added a commit that referenced this issue Jul 11, 2023
Also add PyMapping_GetOptionalItemString() function.
@vstinner
Copy link
Member

I added the 2 functions to pythoncapi-compat: python/pythoncapi-compat@7515dae

serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this issue Sep 2, 2023
The resulting pointer was not set to NULL if the creation of a temporary
string object was failed.

The tests were also missed due to oversight.
serhiy-storchaka added a commit that referenced this issue Sep 6, 2023
The resulting pointer was not set to NULL if the creation of a temporary
string object was failed.

The tests were also missed due to oversight.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic-C-API type-feature A feature request or enhancement
Projects
None yet
Development

No branches or pull requests

3 participants