Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bpo-35568: add 'raise_signal' function #11335

Merged
merged 11 commits into from Jan 8, 2019
Merged

Conversation

vladima
Copy link
Contributor

@vladima vladima commented Dec 28, 2018

As in title, expose C raise function as raise_function in signal module. Also drop existing raise_signal in _testcapi module and replace all usages with new function.

https://bugs.python.org/issue35568

@@ -0,0 +1 @@
Expose :c:func:`raise(signum)` as `raise_signal`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Someone may know better, but I'm pretty sure the :c:func: directive is only for our own C functions, which means this will end up being a broken link.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've used this comment where :c:func: was used for GetFinalPathNameByHandle as an example. If this is not correct - I'd be happy to fix it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zooba's right, :c:func: is used for CPython C API functions. It's a bug if it is used for Posix/etc functions in other places.

Copy link
Contributor Author

@vladima vladima Jan 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it then be just a :func:? It is a bit confusing, Documenting Python doc only mentions c:func as :

c:func
The name of a C-language function. Should include trailing parentheses.

without diving into specific details whether it should be any C function or function from C API. Also I can see a number places in existing docs where externally defined functions are referenced via c:func:, i.e.

*datetime.rst*

if the timestamp is out
   of the range of values supported by the platform C :c:func:`localtime` function
*asyncio-subprocess.rst*

On Windows the Win32 API function :c:func:`TerminateProcess` is
   called to stop the child process.
*asyncore.rst*

If your operating system supports the :c:func:`select` system call in its I/O
library (and nearly all do),

*ctypes.rst*

I will present an example here which uses the standard C library's
:c:func:`qsort` function

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:c:func:qsort creates a link to .. c:function:: qsort block.
If the block is absent no link is created but :c:func:qsort is pointless in this case.
Since CPython documentation has no definition for .. c:function:: qsort the cross-reference should be removed, consider it as a hurtless bug.

The same is for raise. Double backticks should work fine

Modules/signalmodule.c Show resolved Hide resolved
@vladima
Copy link
Contributor Author

vladima commented Jan 4, 2019

is there anything else that should be done for this PR?

Copy link
Member

@1st1 1st1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall (and I'm +1 to add this new function), but please address the comments before merging.

.. function:: raise_signal(signum)

Sends a signal to the executing process. Returns nothing. If a signal handler
is called, the *raise* function shall not return until after the signal handler does.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sends a signal to the executing process.

IMO "to the calling process" reads better.

If a signal handler
is called, the raise function shall not return until after the signal handler does.

While this is true for the C raise function, it's not for Python. In Python, signals are queued and their handlers are executed at some undetermined point later. I suggest removing this sentence.

@@ -0,0 +1 @@
Expose :c:func:`raise(signum)` as `raise_signal`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zooba's right, :c:func: is used for CPython C API functions. It's a bug if it is used for Posix/etc functions in other places.

Copy link
Contributor

@asvetlov asvetlov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Double backticks please:

``raise(signum)``

@asvetlov
Copy link
Contributor

asvetlov commented Jan 8, 2019

Thanks!

@vladima vladima deleted the raise branch November 10, 2020 20:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants