Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork34.2k
gh-145376: Fix various reference leaks objects and modules#145385
gh-145376: Fix various reference leaks objects and modules#145385eendebakpt wants to merge 5 commits intopython:mainfrom
Conversation
StanFromIreland commentedMar 1, 2026
Is this going to be a competition between Claude and Codex now 😆? |
eendebakpt commentedMar 1, 2026 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
As long as the competition is on quality and not quantity we should be fine :-) Update: I checked |
encukou commentedMar 3, 2026
It defintely shouldn't be on quantity of PRs. Could you combine these rather than send several PRs at once? |
eendebakpt commentedMar 3, 2026
I know some other core devs strongly prefer to keep PRs small and isolated. For that reason I created more PRs targeting only a single module at a time. To give an idea: analyzing the Modules folder resulted in 60 potential bugs in 30 files. I estimate that about 20 of them are false positives, or tricky cases. With more prompting to the LLM additional potential issues might be found. Let me know how you would like to group the remaining cases. |
encukou commentedMar 4, 2026
IMO, you should limit the number of similar PRs you have “in flight”, so if a common issue is found in one of them1 you don't need to update all the others. And to focus the review in one place. Footnotes
|
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| static PyObject * | ||
| count_nextlong(countobject *lz) | ||
| { | ||
| PyObject *long_cnt; | ||
| PyObject *stepped_up; | ||
| long_cnt = lz->long_cnt; | ||
| if (long_cnt == NULL) { | ||
| /* Switch to slow_mode */ | ||
| long_cnt = PyLong_FromSsize_t(PY_SSIZE_T_MAX); | ||
| if (long_cnt == NULL) | ||
| if (long_cnt == NULL) { | ||
| return NULL; | ||
| } | ||
| lz->long_cnt = long_cnt; | ||
| } | ||
| assert(lz->cnt == PY_SSIZE_T_MAX && long_cnt != NULL); | ||
| stepped_up = PyNumber_Add(long_cnt, lz->long_step); | ||
| if (stepped_up == NULL) | ||
| return NULL; | ||
| lz->long_cnt = stepped_up; | ||
| return long_cnt; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Is this the way to make it less confusing?
staticPyObject*count_nextlong(countobject*lz){if (lz->long_cnt==NULL) {/* Switch to slow_mode */lz->long_cnt=PyLong_FromSsize_t(PY_SSIZE_T_MAX);if (lz->long_cnt==NULL) {returnNULL; } }assert(lz->cnt==PY_SSIZE_T_MAX&&lz->long_cnt!=NULL);// We hold one reference to "result" (a.k.a. the old value of// lz->long_cnt); we'll either return it or keep it in lz->long_cnt.PyObject*result=lz->long_cnt;PyObject*stepped_up=PyNumber_Add(result,lz->long_step);if (stepped_up==NULL) {returnNULL; }lz->long_cnt=stepped_up;returnresult;}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yeah, this looks less confusing.
Uh oh!
There was an error while loading.Please reload this page.
| @@ -0,0 +1 @@ | |||
| Fix reference leaks in various unusual error scenarios. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I don't think that a NEWS entry is needed since it's unlikely to hit the issue, users are not directly impacted.
Uh oh!
There was an error while loading.Please reload this page.
Found using Claude.