Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34k
gh-119333: Add c-api to have contextvar enter/exit callbacks#119335
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
fried commented May 21, 2024 • edited by github-actions bot
Loading Uh oh!
There was an error while loading. Please reload this page.
edited by github-actions bot
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
1st1 left a comment
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'm OK with this change. The motivation for adding it seems reasonable. I'll let @ericsnowcurrently review it properly, but I skimmed through the code and I like the approach.
erlend-aasland left a comment
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.
Some minor remarks after a quick look: please align the naming with PEP-7 and use Py (not PY) prefixes for the macros.
Uh oh!
There was an error while loading. Please reload this page.
fried commented May 22, 2024
@ericsnowcurrently Am I doing this wrong, Having an issue with Check if generated files are up to date I followed the example for the other watcher checks and they use the same style globals. Also I can't run the check locally it on my mac it doesn't seem to like clang. |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
ericsnowcurrently commented May 22, 2024
Try copying similar entries in Tools/c-analyzer/cpython/ignored.tsv and then editing them to match the new variables.
Yeah, sorry, this is definitely a gap. |
Co-authored-by: Erlend E. Aasland <[email protected]>
Co-authored-by: Erlend E. Aasland <[email protected]>
e2fef0c to c210351CompareUh oh!
There was an error while loading. Please reload this page.
| static void notify_context_watchers(PyContextEvent event, PyContext *ctx) | ||
| { | ||
| assert(Py_REFCNT(ctx) > 0); | ||
| PyInterpreterState *interp = _PyInterpreterState_GET(); |
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.
FWIW you can get interp from tstate which is already known, it would have avoided one more tls read which is slow on msvc.
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.
Sorry. We’ll fix it tomorrow. Any other concerns?
| } PyContextEvent; | ||
| /* | ||
| * A Callback to clue in non-python contexts impls about a |
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 don't understand this comment, what is meant by clue in? Also what is change in active python context?
| * A Callback to clue in non-python contexts impls about a | ||
| * change in the active python context. | ||
| * | ||
| * The callback is invoked with the event and a reference to = |
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.
Remove the =, it adds no useful info and is confusing.
This PR implements gh-119333, allowing for setting a callback function on PyContext_Enter and PyContext_Exit from the c-api
Co-authored-by: @itamaro
📚 Documentation preview 📚: https://cpython-previews--119335.org.readthedocs.build/