Sorry, something went wrong.
|
cc @brandtbucher |
Sorry, something went wrong.
There was a problem hiding this comment.
Thanks for your work on this! Looks good, just one question below:
Sorry, something went wrong.
|
|
||
| #ifdef CLOCK_REALTIME | ||
| PyModule_AddIntMacro(m, CLOCK_REALTIME); | ||
| if (PyModule_AddIntMacro(m, CLOCK_REALTIME) < 0) { |
There was a problem hiding this comment.
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:
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 a problem hiding this comment.
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 a problem hiding this comment.
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 a problem hiding this comment.
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.
https://bugs.python.org/issue1635741