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
Use PyDict_Contains() and PyDict_SetDefault() instead of PyDict_GetItemWithError() #86318
Comments
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:
Disadvantages:
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. |
The code uses also _PyDict_ContainsId() added in bpo-42006 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. |
May I close this issue? |
Sure. Thanks for reminder. |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: