gh-111789: Use PyDict_GetItemRef()#111790
gh-111789: Use PyDict_GetItemRef()#111790serhiy-storchaka wants to merge 11 commits intopython:mainfrom
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.
|
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. |
|
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 owners
|
Split on more PRs. |
serhiy-storchaka
removed request for
1st1,
asvetlov,
gvanrossum,
iritkatriel,
kumaraditya303,
markshannon,
vstinner and
willingc
Is this PR ready for a review? It's marked as a draft and now has a conflict. |
|
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.