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
Updates to PEP-539 draft #248
Conversation
… new-comers to the PEP, as there have also been recent PEPs/discussions surrounding TLS in the HTTPS sense
…link to the relevant bug
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me - my comments are either minor, or else relate to existing PEP content that I noticed was incorrect or unclear while reviewing the update.
I can either wait for you to make further updates, and then merge multiple commits in this PR, or else merge this one, and any subsequent changes can go in a new PR - let me know which you'd prefer.
pep-0539.txt
Outdated
|
||
The new ``PyThread_tss_`` functions are almost exactly analogous to their | ||
original counterparts with a minor difference: Whereas | ||
``PyThread_create_key`` takes no arguments and returns a TLS key as an | ||
``int``, ``PyThread_tss_create`` takes a ``Py_tss_t*`` as an argument, and | ||
returns a ``Py_tss_t`` by pointer--the ``int`` return value is a status, | ||
returns a ``Py_tss_t`` by pointer. The behavior of ``PyThread_tss_create`` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Existing bug in the description here: the new API returns an int
as a status code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I was saying it returns a new Py_tss_t via the pointer (and later I say it returns a status code), but that's confusing and not even quite accurate so I'll reword it.
pep-0539.txt
Outdated
status codes are not defined by this specification. | ||
|
||
Similarly ``PyThread_tss_delete`` is passed a ``Py_tss_t*`` whereas | ||
previouly the key was passed to ``PyThread_delete_key`` by value. | ||
|
||
The old ``PyThread_*_key*`` functions will be marked as deprecated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be explicit as to whether or not they will generate runtime deprecation warnings, or just be marked as deprecated in the documentation (I'm inclined to recommend the latter for now, at least until Python 2.7 goes end-of-life).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same. Let me double-check on what the implementation has (I'm pretty sure also just a documentation deprecation).
pep-0539.txt
Outdated
|
||
The initialization state of the key can then be checked like:: | ||
|
||
assert(PyThread_tss_is_created(tss_key)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably worth including the trailing semi-colon here (I find the absence a bit jarring, even though this doesn't technically have to be a full C statement)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree.
pep-0539.txt
Outdated
} | ||
/* ... once done with the key ... */ | ||
PyThread_tss_delete(&tss_key); | ||
assert(!PyThread_tss_is_created(tss_key)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing trailing semi-colon
pep-0539.txt
Outdated
does not specify a sentintel for an uninitialized ``pthread_key_t``, instead | ||
relying on the ``pthread_once`` interface to ensure that a given TLS key is | ||
initialized only once per-process. Therefore, the ``Py_tss_t`` type | ||
contains an explicit ``._is_initialized`` that can inidicate the key's |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: inidicate
-> indicate
Thanks for the review--I'll just make the updates here. |
Long overdue updates to the PEP-539 draft, summarizing discussion in the original python-ideas thread and updates to the implementation since the last version of the draft.
@ncoghlan @ma8ma