Uh oh!
There was an error while loading. Please reload this page.
gh-111789: Use PyDict_GetItemRef() - #111790
Conversation
vstinner
left a comment
There was a problem hiding this comment.
Reviewing code with 3 cases (error, not found, found) is really hard to me, especially since you also change ownership and reference counting in the same PR. I cannot review such giant single PR. If you want me to review it, please create smaller PRs. For example, only change around 10 PyDict_Get call per PR. Just Modules/_ctypes/_ctypes.c alone is already a big piece.
| } | ||
| int rc = PyDict_GetItemRef(dict, key, presult); | ||
| if (rc <= 0) { // error or not found | ||
| *presult = NULL; |
There was a problem hiding this comment.
Yes, redundant. And the code here can be simplified even more.
| // error or found | ||
| Py_DECREF(key); | ||
| return result; | ||
| } |
There was a problem hiding this comment.
Please add // not found after to be extra explicit, since this code pattern is not common.
There was a problem hiding this comment.
Added here and in other places.
vstinner
commented
Nov 14, 2023
What's the status of that PR? Was it rebased on the main branch to retrieve other already merged changes? It's marked as a draft. I prefer to wait until it's not a draft to review it. |
serhiy-storchaka
commented
Nov 14, 2023
It was merged with The remaining is relatively small and simple. |
serhiy-storchaka
commented
Nov 14, 2023
Split on more PRs. |
vstinner
commented
Jan 10, 2024
Is this PR ready for a review? It's marked as a draft and now has a conflict. |
serhiy-storchaka
commented
Jan 11, 2024
I left the combined PR to see the general image, but most of changes were already merged in other PRs. |
It is a sample. We perhaps will only merge a part of these changes.