Description

serhiy-storchaka

opened on Jul 1, 2023

Issue body actions

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

• gh-106307: Add PyMapping_GetOptionalItem() #106308

Activity

1. serhiy-storchaka

   added

   type-featureA feature request or enhancementA feature request or enhancement

   topic-C-API

   on Jul 1, 2023
2. serhiy-storchaka

   added 2 commits that reference this issue on Jul 1, 2023

   pythongh-106307: Add _PyMapping_LookupItem()

   cfc5851

   pythongh-106307: Add _PyMapping_LookupItem()

   078ea48
3. carljm commented on Jul 3, 2023

   

   carljm

   on Jul 3, 2023

   Member

   More actions

   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?

   Reacted by Erlend E. Aasland
4. serhiy-storchaka commented on Jul 6, 2023

   

   serhiy-storchaka

   on Jul 6, 2023

   MemberAuthor

   More actions

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

   Reacted by Carl Meyer and Erlend E. Aasland
5. serhiy-storchaka

   changed the title [-]Add _PyMapping_LookupItem()[/-] [+]Add PyMapping_GetOptionalItem()[/+] on Jul 7, 2023
6. serhiy-storchaka

   added a commit that references this issue on Jul 11, 2023

   gh-106307: C API: Add PyMapping_GetOptionalItem() function (GH-106308)

   4bf4371
7. serhiy-storchaka

   closed this as completedon Jul 11, 2023
8. vstinner commented on Jul 12, 2023

   

   vstinner

   on Jul 12, 2023

   Member

   More actions

   I added the 2 functions to pythoncapi-compat: python/pythoncapi-compat@7515dae
9. serhiy-storchaka

   added 2 commits that reference this issue on Sep 2, 2023

   pythongh-106307: Fix PyMapping_GetOptionalItemString()

   0d54c80

   gh-106307: Fix PyMapping_GetOptionalItemString() (GH-108797)

   3a08db8

Issue actions