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-121554: remove unnecessary internal functions in compile.c #121555

Merged
merged 8 commits into from
Jul 10, 2024
Merged

gh-121554: remove unnecessary internal functions in compile.c #121555

merged 8 commits into from
Jul 10, 2024

Conversation

iritkatriel
Copy link
Member

@iritkatriel iritkatriel commented Jul 9, 2024 •
edited by bedevere-app bot
Loading

@bedevere-app bedevere-app bot mentioned this pull request Jul 9, 2024
Closed
erlend-aasland
erlend-aasland reviewed Jul 9, 2024
View reviewed changes
Copy link
Contributor

@erlend-aasland erlend-aasland left a comment

Choose a reason for hiding this comment

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

Consider splitting this PR in two:

  1. the build change
  2. the C API change

Include/internal/pycore_compile.h Show resolved Hide resolved
Modules/Setup.bootstrap.in Show resolved Hide resolved
@erlend-aasland
Copy link
Contributor

I wonder if _PyImport_Inittab also needs to change now.

@iritkatriel
Copy link
Member Author

I wonder if _PyImport_Inittab also needs to change now.

It has both:

  {"_opcode", PyInit__opcode},
   {"_operator", PyInit__operator},

so it seems to cover both linkage methods.

@iritkatriel
Copy link
Member Author

Consider splitting this PR in two:

  1. the build change
  2. the C API change

If we need to revert the build change then we also need to revert the code change. Might be easier in one PR.

@erlend-aasland
Copy link
Contributor

Yeah, but _opcode currently comes after the second freeze marker, FWIW.

@iritkatriel
Copy link
Member Author

Yeah, but _opcode currently comes after the second freeze marker, FWIW.

What does that mean?

@erlend-aasland
Copy link
Contributor

Yeah, but _opcode currently comes after the second freeze marker, FWIW.

What does that mean?

Ah, forget it; I was looking at a generated file. Sorry 'bout the noise.

@iritkatriel
Copy link
Member Author

@erlend-aasland Thanks for your help. Tests pass now, are you happy to approve this?

erlend-aasland
erlend-aasland reviewed Jul 10, 2024
View reviewed changes
Modules/_opcode.c Outdated Show resolved Hide resolved
erlend-aasland
erlend-aasland approved these changes Jul 10, 2024
View reviewed changes
Copy link
Contributor

@erlend-aasland erlend-aasland left a comment

Choose a reason for hiding this comment

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

Thanks!

@iritkatriel iritkatriel merged commit 6557af6 into python:main Jul 10, 2024
36 checks passed
noahbkim pushed a commit to hudson-trading/cpython that referenced this pull request Jul 11, 2024
@iritkatriel @erlend-aasland @noahbkim
pythongh-121554: remove unnecessary internal functions in compile.c (p…
2483331 
…ython#121555)

Co-authored-by: Erlend E. Aasland <[email protected]>
estyxx pushed a commit to estyxx/cpython that referenced this pull request Jul 17, 2024
@iritkatriel @erlend-aasland @estyxx
pythongh-121554: remove unnecessary internal functions in compile.c (p…
1041645 
…ython#121555)

Co-authored-by: Erlend E. Aasland <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@erlend-aasland erlend-aasland erlend-aasland approved these changes

@markshannon markshannon Awaiting requested review from markshannon markshannon is a code owner

@corona10 corona10 Awaiting requested review from corona10 corona10 is a code owner

Assignees
No one assigned
Labels
skip news
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

remove _PyCompile_OpcodeHas* functions
2 participants
@iritkatriel @erlend-aasland