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

bpo-43693: Group the code in codeobject.c logically. #26216

Conversation

ericsnowcurrently
Copy link
Member

@ericsnowcurrently ericsnowcurrently commented May 18, 2021

At the moment the code in codeobject.c is a bit scattered around, relative to the logical grouping of the code. This PR addresses that by moving various functions (and types) into logical groups, with a comment dividing them and identifying each group. With this change, subsequent changes I'm planning on making become cleaner, which is the main motivation here.

https://bugs.python.org/issue43693

@gvanrossum
Copy link
Member

Can you confirm that this is a pure refactoring that doesn't change any APIs or semantics? (That's what it sounds like, but there's a lot of code in the diff...)

@ericsnowcurrently
Copy link
Member Author

Yeah, it is almost exclusively only moving code around in the file. The only 2 exceptions are the addition of decode_linetable() and new_linesiterator(), which are just code_getlnotab() and code_linesiterator() (respectively) with tighter signatures.

@gvanrossum
Copy link
Member

Looks like you broke the argument clinic?

@markshannon
Copy link
Member

This is almost certain to cause merge conflicts with work on PEP 657.
We will be removing all the opcache stuff in a month or two (PEP 659) so I wouldn't bother moving that around.

Given that the line number table is likely to grow and change with PEP 657, how about moving that code to it's own file?
@pablogsal thoughts?

@pablogsal
Copy link
Member

Given that the line number table is likely to grow and change with PEP 657, how about moving that code to it's own file?
@pablogsal thoughts?

I think having it in the same file is fine, as it will cause merge conflicts for us in any case. There is some benefit of having fewer compilation units. In the other hand maybe it will result in a better organisation of the code so if you think it makes sense I would be fine too :)

item = PyTuple_GET_ITEM(tup, i);
if (PyUnicode_CheckExact(item)) {
Py_INCREF(item);
static PyTypeObject LineIterator = {
Copy link
Member

Choose a reason for hiding this comment

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

Now that we are here, we could transform these initializers to C99 :)

@gvanrossum
Copy link
Member

Let's not sit on this longer. LGTM, if either Mark or Pablo approves then just land it.

Copy link
Member

@pablogsal pablogsal left a comment

Choose a reason for hiding this comment

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

Looks good for me, but @markshannon should review the function renames

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.

6 participants