bpo-22674: signal: add strsignal()#6017
Conversation
|
Updated with a NEWS entry. |
seirl
force-pushed
the
strsignal
branch
4 times, most recently
from
828fa2e to
66aa723
Compare
|
Updated with a special case for Windows. The tests are all passing now, just waiting for a review. |
There was a problem hiding this comment.
According to POSIX, strsignal returns non-const char*, so let's keep it like that.
There was a problem hiding this comment.
I don't understand this one. Why would we want to keep it not const? We're not planning on modifying it, adding constness to make sure of that doesn't hurt.
There was a problem hiding this comment.
Py_BuildValue doesn't specify that it takes only const arguments, so you may get compiler warnings or even errors depending on compiler settings. The benefit of using const in such a simple function seem rather low, so it's better to drop it iMHO.
There was a problem hiding this comment.
Got it, I just looked at the docs of Py_Buildvalue. I fixed it, thanks.
There was a problem hiding this comment.
According to POSIX, you may also want to check for errno after the call (and set it to zero before).
There was a problem hiding this comment.
Let's not add this kind of post-processing. If someone wants the same return values on all systems, they should simply not use strsignal at all (it's not very hard to come up with descriptions for a dozen or less well-known signals).
There was a problem hiding this comment.
Please use double quotes: this is English prose, not Python :-)
There was a problem hiding this comment.
Note that we indent C code by 4-space indentations, not 2. See PEP 7 for more details.
pitrou
left a comment
pitrou
left a comment
There was a problem hiding this comment.
Thanks for posting this PR! I've added some comments to the code, please make the necessary changes :-)
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase And if you don't make the requested changes, you will be put in the comfy chair! |
|
I have made the requested changes; please review again. |
|
Thanks for making the requested changes! @pitrou: please review the changes made to this pull request. |
Co-authored-by: Vajrasky Kok <sky.kok@speaklikeaking.com>
|
Thank you @seirl ! This looks good to me now. |
|
@pitrou: Please replace |
Co-authored-by: Vajrasky Kok <sky.kok@speaklikeaking.com>
4 participants
I updated @vajrasky 's patch in https://bugs.python.org/issue22674 to rebase it onto master, use the clinic argument parser and improve the docs.
https://bugs.python.org/issue22674