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.

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

gh-87092: move CFG related code from compile.c to flowgraph.c #103021

Merged
merged 15 commits into from
Mar 31, 2023
Merged

gh-87092: move CFG related code from compile.c to flowgraph.c #103021

merged 15 commits into from
Mar 31, 2023

Conversation

iritkatriel
Copy link
Member

@iritkatriel iritkatriel commented Mar 24, 2023 •
edited by bedevere-bot
Loading

@bedevere-bot bedevere-bot mentioned this pull request Mar 24, 2023
Closed
@iritkatriel iritkatriel added skip news interpreter-core (Objects, Python, Grammar, and Parser dirs) labels Mar 24, 2023
@iritkatriel iritkatriel requested a review from a team as a code owner March 25, 2023 18:48
@iritkatriel iritkatriel added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Mar 25, 2023
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @iritkatriel for commit abaeeac 🤖

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Mar 25, 2023
@iritkatriel iritkatriel changed the title gh-87092: move CFG related code from compile.c to cfg.c gh-87092: move CFG related code from compile.c to cfg_opt.c Mar 27, 2023
@iritkatriel iritkatriel changed the title gh-87092: move CFG related code from compile.c to cfg_opt.c gh-87092: move CFG related code from compile.c to flowgraph.c Mar 29, 2023
markshannon
markshannon requested changes Mar 31, 2023
View reviewed changes
Copy link
Member

@markshannon markshannon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bit of duplicated code, otherwise looks fine.

I don't think _PyOpcode_RelativeJump is needed any more, as all jumps are relative.

Include/internal/pycore_compile.h Outdated Show resolved Hide resolved
Include/internal/pycore_opcode_utils.h Outdated Show resolved Hide resolved
Include/internal/pycore_opcode_utils.h Outdated Show resolved Hide resolved
@bedevere-bot
Copy link

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

And if you don't make the requested changes, you will be put in the comfy chair!

@iritkatriel
Copy link
Member Author

I don't think _PyOpcode_RelativeJump is needed any more, as all jumps are relative.

I agree, but I'd like to do it in a separate PR because I would need to remove usages and the diff here is large enough.

@iritkatriel
Copy link
Member Author

I have made the requested changes; please review again.

@bedevere-bot
Copy link

Thanks for making the requested changes!

@markshannon: please review the changes made to this pull request.

@iritkatriel iritkatriel merged commit 80163e1 into python:main Mar 31, 2023
@iritkatriel iritkatriel deleted the cfg.c branch April 3, 2023 17:41
warsaw pushed a commit to warsaw/cpython that referenced this pull request Apr 11, 2023
@iritkatriel @warsaw
pythongh-87092: move CFG related code from compile.c to flowgraph.c (p…
68bbf79 
…ython#103021)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@markshannon markshannon markshannon approved these changes

Assignees
No one assigned
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) skip news
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@iritkatriel @bedevere-bot @markshannon