GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||
GeezerVison is showing github.com — bpo-1635741: Fix refleaks of timemoudle in PyInit_time() by shihai1991 · Pull Request #18486 · python/cpython Conversation
|
|
cc @brandtbucher |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your work on this! Looks good, just one question below:
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
|
||
| #ifdef CLOCK_REALTIME | ||
| PyModule_AddIntMacro(m, CLOCK_REALTIME); | ||
| if (PyModule_AddIntMacro(m, CLOCK_REALTIME) < 0) { |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Is the reason that you need goto in every one of these blocks that PyErr_Occcurred() will get reset if you make other PyModule_AddIntMacro calls that don't fail along the way?
Either way, I'm mildly tempted to suggest adding a wrapper macro like:
#define _PyModule_AddIntMacroOrGotoError(m, macro) \ if (PyModule_AddIntMacro(m, macro) < 0) { goto error; }
But I dunno. I'm torn between the principles of "don't repeat yourself" and "don't write a preprocessor macro with a goto in it (you monster)".
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Between the two options, I'm personally +0 on repetition. Incorrect error handling for the whole PyModule_Add family of functions is rampant in the stdlib (see my work in bpo-38823), and I think reinforcing the correct usage is good to have to keep people from repeating the same old patterns (especially in outside projects).
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Is the reason that you need goto in every one of these blocks that PyErr_Occcurred() will get reset if you make other PyModule_AddIntMacro calls that don't fail along the way?
No, I just try to make the Py_DECREF(m);return NULL; looks like a common processing item.
I think reinforcing the correct usage is good to have to keep people from repeating the same old patterns
+1
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I dislike macros hiding a goto. A goto statement is dangerous, I prefer to make it explicit. The "if (...) { goto error; }" is a very common pattern in Python, especially in initialization code.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Thanks @shihai1991, I merged your PR. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.