Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34k
gh-119786: improve internal docs on co_linetable#123198
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
gh-119786: improve internal docs on co_linetable#123198
Uh oh!
There was an error while loading. Please reload this page.
Conversation
picnixz commented Aug 21, 2024 • edited by bedevere-app bot
Loading Uh oh!
There was an error while loading. Please reload this page.
edited by bedevere-app bot
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
markshannon left a comment • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Up until the section "Artificial constructions" this seems fine.
I don't think we want to be encouraging anyone to create position tables.
The reason for explaining the layout in detail is for anyone maintaining the code.
Even for out of process tools, like pyspy, we provide C functions for them to use, so they don't need to parse the tables.
To create tables for testing dis and such, I'd recommend parsing Python code, modifying the locations on the AST, then generating the code object with compile.
Like this:
importastimportdiscode="""def foo(): a = 1 b = 2 c = 3"""tree=ast.parse(code) func=tree.body[0] b=func.body[1].targets[0] b.lineno=5b.col_offset=17b.end_lineno=6b.end_col_offset=18foo=compile(tree,"test", "exec") dis.dis(foo.co_consts[0], show_positions=True)2:0-2:0 RESUME 0 3:8-3:9 LOAD_CONST 1 (1) 3:4-3:5 STORE_FAST 0 (a) 4:8-4:9 LOAD_CONST 2 (2) 5:17-6:18 STORE_FAST 1 (b) 5:8-5:9 LOAD_CONST 3 (3) 5:4-5:5 STORE_FAST 2 (c) 5:4-5:5 RETURN_CONST 0 (None) picnixz commented Aug 21, 2024
Oh, that's what I was searching for in #123168. Should I amend this PR and use your approach instead? |
iritkatriel commented Aug 21, 2024
Yes, make a new PR though as that one was merged. |
picnixz commented Aug 21, 2024
Actually, modifying the AST does not help because I can't remove some attributes that are expected to exist on the nodes. In particular, I cannot cancel positions (but I can easily change the values of the positions). So I think we need to stick with the |
markshannon commented Aug 22, 2024
I see. You can't set an expression to have no location, because the compiler will just give it one. I think the test you wrote is fine. There is need to rewrite it. |
picnixz commented Aug 22, 2024
Yes, that's what I saw. But I'm not sure whether this is actually a bug or a feature of the compiler (namely, whether the -1 is really meant to indicate "no location"). |
picnixz commented Aug 22, 2024
So I managed to use AST tricks only though I'm not really sure that setting the columns to -1 means that we are deleting the information. I'll remove the artificial constructions in these docs anyway. |
co_linetableco_linetablepicnixz commented Nov 27, 2024 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
Friendly ping @iritkatriel (I'll fix the merge conflicts once I'm back, sorry missed that one) |
iritkatriel commented Nov 27, 2024
There's a merge conflict now. |
picnixz commented Nov 27, 2024
Since I'm on mobile I cannot resolve it but I'll take care of it on Friday (or maybe you can if you have time). Sorry for bothering you! |
picnixz commented Nov 29, 2024
@iritkatriel Ok, actually the |
49f15d8 into python:mainUh oh!
There was an error while loading. Please reload this page.
iritkatriel commented Nov 30, 2024
Thank you! |
In #123168, I needed to play with artificial instructions where some positions are not available. It's trivial to have full positions information or no positions information, but it's hard to create
co_linetablevalues where only some instructions have positions.This PR aims to improve the internal docs so that we can easily remember what to do (and how to do it with an example).