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. |
Modules/signalmodule.c
Outdated
| /*[clinic end generated code: output=44e12e1e3b666261 input=5369187c5b204b2a]*/ | ||
| { | ||
| int position; | ||
| const char *res, *pch; |
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.
Modules/signalmodule.c
Outdated
| #else | ||
| res = strsignal(signalnum); | ||
|
|
||
| if (res == NULL || strstr(res, "Unknown signal") != NULL) |
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).
Modules/signalmodule.c
Outdated
| if (res == NULL || strstr(res, "Unknown signal") != NULL) | ||
| Py_RETURN_NONE; | ||
|
|
||
| /* On Mac, it returns 'Quit: 3', while on Linux it returns 'Quit'. */ |
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).
Doc/library/signal.rst
Outdated
| .. function:: strsignal(signalnum) | ||
|
|
||
| Return the system description of the signal *signalnum*, such as | ||
| 'Interrupt', 'Segmentation fault', etc. Returns :const:`None` if the signal |
There was a problem hiding this comment.
Please use double quotes: this is English prose, not Python :-)
Modules/signalmodule.c
Outdated
| res = strsignal(signalnum); | ||
|
|
||
| if (res == NULL || strstr(res, "Unknown signal") != NULL) | ||
| Py_RETURN_NONE; |
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.
|
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>
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