8000 [3.11] GH-96636: Remove all uses of NOTRACE_DISPATCH (GH-96643) by markshannon · Pull Request #96688 · python/cpython · GitHub
[go: up one dir, main page]
Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Merged
gvanrossum merged 2 commits into from
Sep 9, 2022
Merged

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

gvanrossum merged 2 commits into from
Sep 9, 2022

Conversation

@markshannon
Copy link
Member
@markshannon markshannon commented Sep 8, 2022 •
edited by bedevere-bot
Loading

Co-authored-by: Brandt Bucher brandtbucher@gmail.com
(cherry picked from commit aa3b4cf)

markshannon and others added 2 commits September 8, 2022 17:34
@markshannon @brandtbucher
pythonGH-96636: Remove all uses of NOTRACE_DISPATCH (pythonGH-96643)
c3e987b 
Co-authored-by: Brandt Bucher <brandtbucher@gmail.com>
(cherry picked from commit aa3b4cf)
@markshannon
Remove merge artiifacts.
993bf61
@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
@bedevere-bot bedevere-bot mentioned this pull request Sep 8, 2022
Merged
@gvanrossum
Copy link
Member
gvanrossum commented Sep 8, 2022

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

@markshannon
Copy link
Member Author
markshannon commented Sep 9, 2022

@pablogsal?

@pablogsal
Copy link
Member
pablogsal commented Sep 9, 2022

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
markshannon commented Sep 9, 2022

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
pablogsal commented Sep 9, 2022

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
brandtbucher commented Sep 21, 2022

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

Reviewers

@gvanrossum gvanrossum gvanrossum approved these changes

@pablogsal pablogsal pablogsal approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@markshannon @gvanrossum @pablogsal @nedbat @neonene @brandtbucher @bedevere-bot
0