From b510e101f8b5b31276bf97b921ca9247162881d2 Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Mon, 26 Oct 2020 12:47:57 +0200 Subject: [PATCH] bpo-42152: Use PyDict_Contains and PyDict_SetDefault if appropriate. (GH-22986) If PyDict_GetItemWithError is only used to check whether the key is in dict, it is better to use PyDict_Contains instead. And if it is used in combination with PyDict_SetItem, PyDict_SetDefault can replace the combination. --- Modules/_ctypes/_ctypes.c | 21 +++---- Modules/_ctypes/callbacks.c | 5 +- Modules/_pickle.c | 21 +++---- Modules/posixmodule.c | 12 ++-- Modules/pyexpat.c | 10 +-- Modules/selectmodule.c | 13 ++-- Objects/dictobject.c | 45 +++++++------ Objects/typeobject.c | 122 +++++++++++++++--------------------- Python/bltinmodule.c | 20 +++--- Python/errors.c | 9 +-- Python/import.c | 24 ++++--- 11 files changed, 137 insertions(+), 165 deletions(-) diff --git a/Modules/_ctypes/_ctypes.c b/Modules/_ctypes/_ctypes.c index 0ac48b92bff..9be90eb27bd 100644 --- a/Modules/_ctypes/_ctypes.c +++ b/Modules/_ctypes/_ctypes.c @@ -492,9 +492,10 @@ StructUnionType_new(PyTypeObject *type, PyObject *args, PyObject *kwds, int isSt return NULL; /* keep this for bw compatibility */ - if (_PyDict_GetItemIdWithError(result->tp_dict, &PyId__abstract_)) + int r = _PyDict_ContainsId(result->tp_dict, &PyId__abstract_); + if (r > 0) return (PyObject *)result; - if (PyErr_Occurred()) { + if (r < 0) { Py_DECREF(result); return NULL; } @@ -4397,15 +4398,13 @@ _init_pos_args(PyObject *self, PyTypeObject *type, } val = PyTuple_GET_ITEM(args, i + index); if (kwds) { - if (PyDict_GetItemWithError(kwds, name)) { - PyErr_Format(PyExc_TypeError, - "duplicate values for field %R", - name); - Py_DECREF(pair); - Py_DECREF(name); - return -1; - } - else if (PyErr_Occurred()) { + res = PyDict_Contains(kwds, name); + if (res != 0) { + if (res > 0) { + PyErr_Format(PyExc_TypeError, + "duplicate values for field %R", + name); + } Py_DECREF(pair); Py_DECREF(name); return -1; diff --git a/Modules/_ctypes/callbacks.c b/Modules/_ctypes/callbacks.c index 2abfa67cdc0..5cd85772485 100644 --- a/Modules/_ctypes/callbacks.c +++ b/Modules/_ctypes/callbacks.c @@ -109,8 +109,9 @@ TryAddRef(StgDictObject *dict, CDataObject *obj) IUnknown *punk; _Py_IDENTIFIER(_needs_com_addref_); - if (!_PyDict_GetItemIdWithError((PyObject *)dict, &PyId__needs_com_addref_)) { - if (PyErr_Occurred()) { + int r = _PyDict_ContainsId((PyObject *)dict, &PyId__needs_com_addref_); + if (r <= 0) { + if (r < 0) { PrintError("getting _needs_com_addref_"); } return; diff --git a/Modules/_pickle.c b/Modules/_pickle.c index bddd8f46f0e..ed8afefe4c7 100644 --- a/Modules/_pickle.c +++ b/Modules/_pickle.c @@ -2004,26 +2004,21 @@ fast_save_enter(PicklerObject *self, PyObject *obj) self->fast_nesting = -1; return 0; } - if (PyDict_GetItemWithError(self->fast_memo, key)) { - Py_DECREF(key); + int r = PyDict_Contains(self->fast_memo, key); + if (r > 0) { PyErr_Format(PyExc_ValueError, "fast mode: can't pickle cyclic objects " "including object type %.200s at %p", Py_TYPE(obj)->tp_name, obj); - self->fast_nesting = -1; - return 0; } - if (PyErr_Occurred()) { - Py_DECREF(key); - self->fast_nesting = -1; - return 0; - } - if (PyDict_SetItem(self->fast_memo, key, Py_None) < 0) { - Py_DECREF(key); - self->fast_nesting = -1; - return 0; + else if (r == 0) { + r = PyDict_SetItem(self->fast_memo, key, Py_None); } Py_DECREF(key); + if (r != 0) { + self->fast_nesting = -1; + return 0; + } } return 1; } diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index 6ce0bcb9fe8..ccd64d63dd0 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -1526,13 +1526,11 @@ convertenviron(void) Py_DECREF(d); return NULL; } - if (PyDict_GetItemWithError(d, k) == NULL) { - if (PyErr_Occurred() || PyDict_SetItem(d, k, v) != 0) { - Py_DECREF(v); - Py_DECREF(k); - Py_DECREF(d); - return NULL; - } + if (PyDict_SetDefault(d, k, v) == NULL) { + Py_DECREF(v); + Py_DECREF(k); + Py_DECREF(d); + return NULL; } Py_DECREF(k); Py_DECREF(v); diff --git a/Modules/pyexpat.c b/Modules/pyexpat.c index 12ae66d945b..73ea51385ee 100644 --- a/Modules/pyexpat.c +++ b/Modules/pyexpat.c @@ -1614,15 +1614,7 @@ static int init_handler_descrs(void) if (descr == NULL) return -1; - if (PyDict_GetItemWithError(Xmlparsetype.tp_dict, PyDescr_NAME(descr))) { - Py_DECREF(descr); - continue; - } - else if (PyErr_Occurred()) { - Py_DECREF(descr); - return -1; - } - if (PyDict_SetItem(Xmlparsetype.tp_dict, PyDescr_NAME(descr), descr) < 0) { + if (PyDict_SetDefault(Xmlparsetype.tp_dict, PyDescr_NAME(descr), descr) == NULL) { Py_DECREF(descr); return -1; } diff --git a/Modules/selectmodule.c b/Modules/selectmodule.c index fe852f93c37..d02e3905f57 100644 --- a/Modules/selectmodule.c +++ b/Modules/selectmodule.c @@ -499,11 +499,14 @@ select_poll_modify_impl(pollObject *self, int fd, unsigned short eventmask) key = PyLong_FromLong(fd); if (key == NULL) return NULL; - if (PyDict_GetItemWithError(self->dict, key) == NULL) { - if (!PyErr_Occurred()) { - errno = ENOENT; - PyErr_SetFromErrno(PyExc_OSError); - } + err = PyDict_Contains(self->dict, key); + if (err < 0) { + Py_DECREF(key); + return NULL; + } + if (err == 0) { + errno = ENOENT; + PyErr_SetFromErrno(PyExc_OSError); Py_DECREF(key); return NULL; } diff --git a/Objects/dictobject.c b/Objects/dictobject.c index 00d6ab3be2f..faa8696153c 100644 --- a/Objects/dictobject.c +++ b/Objects/dictobject.c @@ -2544,8 +2544,8 @@ PyDict_MergeFromSeq2(PyObject *d, PyObject *seq2, int override) goto Fail; } } - else if (PyDict_GetItemWithError(d, key) == NULL) { - if (PyErr_Occurred() || PyDict_SetItem(d, key, value) < 0) { + else { + if (PyDict_SetDefault(d, key, value) == NULL) { Py_DECREF(key); Py_DECREF(value); goto Fail; @@ -2660,19 +2660,20 @@ dict_merge(PyObject *a, PyObject *b, int override) Py_INCREF(value); if (override == 1) err = insertdict(mp, key, hash, value); - else if (_PyDict_GetItem_KnownHash(a, key, hash) == NULL) { - if (PyErr_Occurred()) { - Py_DECREF(value); - Py_DECREF(key); - return -1; + else { + err = _PyDict_Contains_KnownHash(a, key, hash); + if (err == 0) { + err = insertdict(mp, key, hash, value); + } + else if (err > 0) { + if (override != 0) { + _PyErr_SetKeyError(key); + Py_DECREF(value); + Py_DECREF(key); + return -1; + } + err = 0; } - err = insertdict(mp, key, hash, value); - } - else if (override != 0) { - _PyErr_SetKeyError(key); - Py_DECREF(value); - Py_DECREF(key); - return -1; } Py_DECREF(value); Py_DECREF(key); @@ -2709,17 +2710,15 @@ dict_merge(PyObject *a, PyObject *b, int override) for (key = PyIter_Next(iter); key; key = PyIter_Next(iter)) { if (override != 1) { - if (PyDict_GetItemWithError(a, key) != NULL) { - if (override != 0) { + status = PyDict_Contains(a, key); + if (status != 0) { + if (status > 0) { + if (override == 0) { + Py_DECREF(key); + continue; + } _PyErr_SetKeyError(key); - Py_DECREF(key); - Py_DECREF(iter); - return -1; } - Py_DECREF(key); - continue; - } - else if (PyErr_Occurred()) { Py_DECREF(key); Py_DECREF(iter); return -1; diff --git a/Objects/typeobject.c b/Objects/typeobject.c index 6626169ae37..bd1587ace87 100644 --- a/Objects/typeobject.c +++ b/Objects/typeobject.c @@ -2388,7 +2388,7 @@ type_new(PyTypeObject *metatype, PyObject *args, PyObject *kwds) PyHeapTypeObject *et; PyMemberDef *mp; Py_ssize_t i, nbases, nslots, slotoffset, name_size; - int j, may_add_dict, may_add_weak, add_dict, add_weak; + int j, r, may_add_dict, may_add_weak, add_dict, add_weak; _Py_IDENTIFIER(__qualname__); _Py_IDENTIFIER(__slots__); _Py_IDENTIFIER(__classcell__); @@ -2542,7 +2542,12 @@ type_new(PyTypeObject *metatype, PyObject *args, PyObject *kwds) goto error; } PyList_SET_ITEM(newslots, j, tmp); - if (PyDict_GetItemWithError(dict, tmp)) { + r = PyDict_Contains(dict, tmp); + if (r < 0) { + Py_DECREF(newslots); + goto error; + } + if (r > 0) { /* CPython inserts __qualname__ and __classcell__ (when needed) into the namespace when creating a class. They will be deleted below so won't act as class variables. */ @@ -2555,10 +2560,6 @@ type_new(PyTypeObject *metatype, PyObject *args, PyObject *kwds) goto error; } } - else if (PyErr_Occurred()) { - Py_DECREF(newslots); - goto error; - } j++; } assert(j == nslots - add_dict - add_weak); @@ -2643,10 +2644,11 @@ type_new(PyTypeObject *metatype, PyObject *args, PyObject *kwds) type->tp_dict = dict; /* Set __module__ in the dict */ - if (_PyDict_GetItemIdWithError(dict, &PyId___module__) == NULL) { - if (PyErr_Occurred()) { - goto error; - } + r = _PyDict_ContainsId(dict, &PyId___module__); + if (r < 0) { + goto error; + } + if (r == 0) { tmp = PyEval_GetGlobals(); if (tmp != NULL) { tmp = _PyDict_GetItemIdWithError(tmp, &PyId___name__); @@ -2885,6 +2887,7 @@ PyType_FromModuleAndSpec(PyObject *module, PyType_Spec *spec, PyObject *bases) PyHeapTypeObject *res; PyObject *modname; PyTypeObject *type, *base; + int r; const PyType_Slot *slot; Py_ssize_t nmembers, weaklistoffset, dictoffset, vectorcalloffset; @@ -3052,9 +3055,9 @@ PyType_FromModuleAndSpec(PyObject *module, PyType_Spec *spec, PyObject *bases) PyObject *__doc__ = PyUnicode_FromString(_PyType_DocWithoutSignature(type->tp_name, type->tp_doc)); if (!__doc__) goto fail; - int ret = _PyDict_SetItemId(type->tp_dict, &PyId___doc__, __doc__); + r = _PyDict_SetItemId(type->tp_dict, &PyId___doc__, __doc__); Py_DECREF(__doc__); - if (ret < 0) + if (r < 0) goto fail; } @@ -3070,21 +3073,21 @@ PyType_FromModuleAndSpec(PyObject *module, PyType_Spec *spec, PyObject *bases) } /* Set type.__module__ */ - if (_PyDict_GetItemIdWithError(type->tp_dict, &PyId___module__) == NULL) { - if (PyErr_Occurred()) { - goto fail; - } + r = _PyDict_ContainsId(type->tp_dict, &PyId___module__); + if (r < 0) { + goto fail; + } + if (r == 0) { s = strrchr(spec->name, '.'); if (s != NULL) { - int err; modname = PyUnicode_FromStringAndSize( spec->name, (Py_ssize_t)(s - spec->name)); if (modname == NULL) { goto fail; } - err = _PyDict_SetItemId(type->tp_dict, &PyId___module__, modname); + r = _PyDict_SetItemId(type->tp_dict, &PyId___module__, modname); Py_DECREF(modname); - if (err != 0) + if (r != 0) goto fail; } else { if (PyErr_WarnFormat(PyExc_DeprecationWarning, 1, @@ -5035,26 +5038,16 @@ add_methods(PyTypeObject *type, PyMethodDef *meth) } if (!(meth->ml_flags & METH_COEXIST)) { - if (PyDict_GetItemWithError(dict, name)) { - if (!isdescr) { - Py_DECREF(name); - } - Py_DECREF(descr); - continue; - } - else if (PyErr_Occurred()) { - if (!isdescr) { - Py_DECREF(name); - } - return -1; - } + err = PyDict_SetDefault(dict, name, descr) == NULL; + } + else { + err = PyDict_SetItem(dict, name, descr) < 0; } - err = PyDict_SetItem(dict, name, descr); if (!isdescr) { Py_DECREF(name); } Py_DECREF(descr); - if (err < 0) + if (err) return -1; } return 0; @@ -5070,15 +5063,7 @@ add_members(PyTypeObject *type, PyMemberDef *memb) if (descr == NULL) return -1; - if (PyDict_GetItemWithError(dict, PyDescr_NAME(descr))) { - Py_DECREF(descr); - continue; - } - else if (PyErr_Occurred()) { - Py_DECREF(descr); - return -1; - } - if (PyDict_SetItem(dict, PyDescr_NAME(descr), descr) < 0) { + if (PyDict_SetDefault(dict, PyDescr_NAME(descr), descr) == NULL) { Py_DECREF(descr); return -1; } @@ -5097,15 +5082,7 @@ add_getset(PyTypeObject *type, PyGetSetDef *gsp) if (descr == NULL) return -1; - if (PyDict_GetItemWithError(dict, PyDescr_NAME(descr))) { - Py_DECREF(descr); - continue; - } - else if (PyErr_Occurred()) { - Py_DECREF(descr); - return -1; - } - if (PyDict_SetItem(dict, PyDescr_NAME(descr), descr) < 0) { + if (PyDict_SetDefault(dict, PyDescr_NAME(descr), descr) == NULL) { Py_DECREF(descr); return -1; } @@ -5553,10 +5530,11 @@ PyType_Ready(PyTypeObject *type) /* if the type dictionary doesn't contain a __doc__, set it from the tp_doc slot. */ - if (_PyDict_GetItemIdWithError(type->tp_dict, &PyId___doc__) == NULL) { - if (PyErr_Occurred()) { - goto error; - } + int r = _PyDict_ContainsId(type->tp_dict, &PyId___doc__); + if (r < 0) { + goto error; + } + if (r == 0) { if (type->tp_doc != NULL) { const char *old_doc = _PyType_DocWithoutSignature(type->tp_name, type->tp_doc); @@ -5582,10 +5560,12 @@ PyType_Ready(PyTypeObject *type) This signals that __hash__ is not inherited. */ if (type->tp_hash == NULL) { - if (_PyDict_GetItemIdWithError(type->tp_dict, &PyId___hash__) == NULL) { - if (PyErr_Occurred() || - _PyDict_SetItemId(type->tp_dict, &PyId___hash__, Py_None) < 0) - { + r = _PyDict_ContainsId(type->tp_dict, &PyId___hash__); + if (r < 0) { + goto error; + } + if (r == 0) { + if (_PyDict_SetItemId(type->tp_dict, &PyId___hash__, Py_None) < 0) { goto error; } type->tp_hash = PyObject_HashNotImplemented; @@ -6270,19 +6250,17 @@ add_tp_new_wrapper(PyTypeObject *type) { PyObject *func; - if (_PyDict_GetItemIdWithError(type->tp_dict, &PyId___new__) != NULL) + int r = _PyDict_ContainsId(type->tp_dict, &PyId___new__); + if (r > 0) return 0; - if (PyErr_Occurred()) + if (r < 0) return -1; func = PyCFunction_NewEx(tp_new_methoddef, (PyObject *)type, NULL); if (func == NULL) return -1; - if (_PyDict_SetItemId(type->tp_dict, &PyId___new__, func)) { - Py_DECREF(func); - return -1; - } + r = _PyDict_SetItemId(type->tp_dict, &PyId___new__, func); Py_DECREF(func); - return 0; + return r; } /* Slot wrappers that call the corresponding __foo__ slot. See comments @@ -7795,10 +7773,11 @@ recurse_down_subclasses(PyTypeObject *type, PyObject *name, /* Avoid recursing down into unaffected classes */ dict = subclass->tp_dict; if (dict != NULL && PyDict_Check(dict)) { - if (PyDict_GetItemWithError(dict, name) != NULL) { + int r = PyDict_Contains(dict, name); + if (r > 0) { continue; } - if (PyErr_Occurred()) { + if (r < 0) { return -1; } } @@ -7853,9 +7832,10 @@ add_operators(PyTypeObject *type) ptr = slotptr(type, p->offset); if (!ptr || !*ptr) continue; - if (PyDict_GetItemWithError(dict, p->name_strobj)) + int r = PyDict_Contains(dict, p->name_strobj); + if (r > 0) continue; - if (PyErr_Occurred()) { + if (r < 0) { return -1; } if (*ptr == (void *)PyObject_HashNotImplemented) { diff --git a/Python/bltinmodule.c b/Python/bltinmodule.c index 89b7fce8f4a..1ce55b6ec5a 100644 --- a/Python/bltinmodule.c +++ b/Python/bltinmodule.c @@ -924,12 +924,12 @@ builtin_eval_impl(PyObject *module, PyObject *source, PyObject *globals, return NULL; } - if (_PyDict_GetItemIdWithError(globals, &PyId___builtins__) == NULL) { - if (_PyDict_SetItemId(globals, &PyId___builtins__, - PyEval_GetBuiltins()) != 0) - return NULL; + int r = _PyDict_ContainsId(globals, &PyId___builtins__); + if (r == 0) { + r = _PyDict_SetItemId(globals, &PyId___builtins__, + PyEval_GetBuiltins()); } - else if (PyErr_Occurred()) { + if (r < 0) { return NULL; } @@ -1012,12 +1012,12 @@ builtin_exec_impl(PyObject *module, PyObject *source, PyObject *globals, Py_TYPE(locals)->tp_name); return NULL; } - if (_PyDict_GetItemIdWithError(globals, &PyId___builtins__) == NULL) { - if (_PyDict_SetItemId(globals, &PyId___builtins__, - PyEval_GetBuiltins()) != 0) - return NULL; + int r = _PyDict_ContainsId(globals, &PyId___builtins__); + if (r == 0) { + r = _PyDict_SetItemId(globals, &PyId___builtins__, + PyEval_GetBuiltins()); } - else if (PyErr_Occurred()) { + if (r < 0) { return NULL; } diff --git a/Python/errors.c b/Python/errors.c index 02cf47992b6..f80ae21fdde 100644 --- a/Python/errors.c +++ b/Python/errors.c @@ -1098,10 +1098,11 @@ PyErr_NewException(const char *name, PyObject *base, PyObject *dict) goto failure; } - if (_PyDict_GetItemIdWithError(dict, &PyId___module__) == NULL) { - if (_PyErr_Occurred(tstate)) { - goto failure; - } + int r = _PyDict_ContainsId(dict, &PyId___module__); + if (r < 0) { + goto failure; + } + if (r == 0) { modulename = PyUnicode_FromStringAndSize(name, (Py_ssize_t)(dot-name)); if (modulename == NULL) diff --git a/Python/import.c b/Python/import.c index 26b80f320c3..b79bda058db 100644 --- a/Python/import.c +++ b/Python/import.c @@ -1018,14 +1018,14 @@ module_dict_for_exec(PyThreadState *tstate, PyObject *name) /* If the module is being reloaded, we get the old module back and re-use its dict to exec the new code. */ d = PyModule_GetDict(m); - if (_PyDict_GetItemIdWithError(d, &PyId___builtins__) == NULL) { - if (_PyErr_Occurred(tstate) || - _PyDict_SetItemId(d, &PyId___builtins__, - PyEval_GetBuiltins()) != 0) - { - remove_module(tstate, name); - return NULL; - } + int r = _PyDict_ContainsId(d, &PyId___builtins__); + if (r == 0) { + r = _PyDict_SetItemId(d, &PyId___builtins__, + PyEval_GetBuiltins()); + } + if (r < 0) { + remove_module(tstate, name); + return NULL; } return d; /* Return a borrowed reference. */ @@ -1660,10 +1660,14 @@ resolve_name(PyThreadState *tstate, PyObject *name, PyObject *globals, int level goto error; } - if (_PyDict_GetItemIdWithError(globals, &PyId___path__) == NULL) { + int haspath = _PyDict_ContainsId(globals, &PyId___path__); + if (haspath < 0) { + goto error; + } + if (!haspath) { Py_ssize_t dot; - if (_PyErr_Occurred(tstate) || PyUnicode_READY(package) < 0) { + if (PyUnicode_READY(package) < 0) { goto error; }