gh-111789: Use PyDict_GetItemRef()#111790
Uh oh!
There was an error while loading. Please reload this page.
Conversation
vstinner left a comment
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.
Modules/_ctypes/_ctypes.c Outdated
| } | ||
| 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 requested review from 1st1, asvetlov, gvanrossum, iritkatriel, kumaraditya303, markshannon and willingc as code ownersserhiy-storchaka commented Nov 14, 2023
Split on more PRs. |
serhiy-storchaka removed request for 1st1, asvetlov, gvanrossum, iritkatriel, kumaraditya303, markshannon, vstinner and willingcvstinner 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. |
2 participants
It is a sample. We perhaps will only merge a part of these changes.