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

Remove unused DPRINTF in ceval.c #129282

Merged
merged 1 commit into from
Jan 25, 2025

Conversation

chris-eibl
Copy link
Contributor

@chris-eibl chris-eibl commented Jan 25, 2025

Per encouragement from @Fidget-Spinner in #129113 (comment).

DPRINTF was introduced in 7e135a4, but is currently unused.

Alternatively, I can modify it to use frame->lltrace, since lltrace has been moved into the frame (#129113).

I think this is a skip news?

Copy link

cpython-cla-bot bot commented Jan 25, 2025

All commit authors signed the Contributor License Agreement.
CLA signed

@bedevere-app
Copy link

bedevere-app bot commented Jan 25, 2025

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

@chris-eibl
Copy link
Contributor Author

PS: I've stumbled over the immediately following lines

    ; // dummy statement after a label, before a declaration
    uint16_t uopcode;

where I think the first can be removed and the second dedented?

@Fidget-Spinner
Copy link
Member

This shouldn't be tied to the tail call issue, as it's an unrelated improvement. Otherwise everything LGTM. Thanks!. I'll just de-link it and approve it.

@Fidget-Spinner Fidget-Spinner changed the title gh-128563: remove unused DPRINTF in ceval.c Remove unused DPRINTF in ceval.c Jan 25, 2025
@Fidget-Spinner Fidget-Spinner merged commit 8fecb9f into python:main Jan 25, 2025
46 checks passed
@Fidget-Spinner
Copy link
Member

Woops forgot to submit the approval, but I did look through it before merging. Sorry!

@chris-eibl
Copy link
Contributor Author

PS: I've stumbled over the immediately following lines

    ; // dummy statement after a label, before a declaration
    uint16_t uopcode;

where I think the first can be removed and the second dedented?

Is this worth a PR? If so, I'd happily create one. Most probably again a skip news and no issue needed?

@chris-eibl chris-eibl deleted the DPRINTF_in_ceval branch January 25, 2025 18:23
@Fidget-Spinner
Copy link
Member

I think that's used in the tier 2 interpreter?

@chris-eibl
Copy link
Contributor Author

chris-eibl commented Jan 26, 2025

Those two lines are in global scope, so IMHO the ; does not serve any purpose (any longer).

Maybe that's a leftover, when those two lines were part of an #if, but I think ; can be safely removed and the other dedented. Doing so, ceval.c compiles fine for me.

Edited: please scratch that, that's part of _Py_TIER2, which I had not defined when compiling ceval.c :(

@chris-eibl
Copy link
Contributor Author

Oh, sorry, my bad, I think now understand. That belongs to

// Tier 2 is also here!
enter_tier_two:

about 25 lines above, in case _Py_JIT is not defined. Was too hard too read for me with all the macro definitions in between :)

Sorry for the noise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants