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

[3.11] GH-96636: Remove all uses of NOTRACE_DISPATCH (GH-96643) #96688

Merged
merged 2 commits into from
Sep 9, 2022

Conversation

markshannon
Copy link
Member

@markshannon markshannon commented Sep 8, 2022

Co-authored-by: Brandt Bucher [email protected]
(cherry picked from commit aa3b4cf)

@markshannon markshannon changed the title GH-96636: Remove all uses of NOTRACE_DISPATCH (GH-96643) [3.11] GH-96636: Remove all uses of NOTRACE_DISPATCH (GH-96643) Sep 8, 2022
@gvanrossum
Copy link
Member

Whoops this probably should wait for @pablogsal before it gets merged.

@markshannon
Copy link
Member Author

@pablogsal?

@pablogsal
Copy link
Member

It would be great if we can quickly check that the coverage test suite passes or doesn't fail more with this PR before landing.

Otherwise, LGTM, land when ready 👍

@markshannon
Copy link
Member Author

This shouldn't impact coverage.py. This is for weird code that turns on tracing in finalizers, weakrefs or asynchronously.

I'm fairly sure coverage.py doesn't do any of those things. @nedbat?

@nedbat
Copy link
Member

nedbat commented Sep 9, 2022

Coverage.py doesn't do any of those weird things :) Also, I now have nightly build testing every day, so we'll hear soon if this broke something.

@pablogsal
Copy link
Member

Ok, let's merge it then 👍

@gvanrossum gvanrossum merged commit 5586da6 into python:3.11 Sep 9, 2022
@gvanrossum gvanrossum deleted the backport-aa3b4cf-3.11 branch September 9, 2022 16:24
@neonene
Copy link
Contributor

neonene commented Sep 21, 2022

An extra DISPATCH() macro has been added accidentally, which has no harm.

cpython/Python/ceval.c

Lines 3582 to 3586 in 46f791d

DISPATCH();
}
DISPATCH();
TARGET(STORE_ATTR_ADAPTIVE) {

@brandtbucher
Copy link
Member

Nice catch. I'll open a PR to fix.

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