Skip to content

bpo-35568: add 'raise_signal' function#11335

Merged
miss-islington merged 11 commits intopython:masterfrom
vladima:raise
Jan 8, 2019
Merged

bpo-35568: add 'raise_signal' function#11335
miss-islington merged 11 commits intopython:masterfrom
vladima:raise

Conversation

@vladima
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

Comment thread Modules/signalmodule.c
@vladima
Copy link
Copy Markdown
Contributor Author

vladima commented Jan 4, 2019

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

@vladima vladima requested review from 1st1 and asvetlov as code owners January 6, 2019 00:50
Copy link
Copy Markdown
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.

Comment thread Doc/library/signal.rst Outdated
.. 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
Copy Markdown
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
Copy Markdown
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.

Comment thread Doc/library/signal.rst
Copy link
Copy Markdown
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
Copy Markdown
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.

7 participants