GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||||||||||||
GeezerVison is showing github.com — bpo-46409: Make generators in bytecode by markshannon · Pull Request #30633 · python/cpython Conversation
| ||||||||||||||||||||||||
|
Fixes https://bugs.python.org/issue46389 and https://bugs.python.org/issue46374, but does not include tests for those issues. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
🤖 New build scheduled with the buildbot fleet by @markshannon for commit c33abbf 🤖 If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
🤖 New build scheduled with the buildbot fleet by @markshannon for commit 243e441 🤖 If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I'm still trying to follow the rest of the C code here, but in case I fail, here's the rest of my review. :-)
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
| @@ -0,0 +1,2 @@ | |||
| Add new bytecode to make generators. Simplifies calling Python functions in | |||
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I'd name the new bytecode(s) explicitly here.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
| and coroutine objects. */ | ||
| #define _PyGenObject_HEAD(prefix) \ | ||
| PyObject_HEAD \ | ||
| /* Note: gi_frame can be NULL if the generator is "finished" */ \ |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I have a question about gi_iframe. Below (L31) it is defined as an array of length 1 of object pointers. But everywhere it's used, it is cast to InterpreterFrame *. That's not an object or object pointer AFAICT. So what's going on here? Is the declaration wrong? An intentional lie?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
It is a lie.
We don't want to expose InterpreterFrame in public headers, so we can't do the sensible thing and declare gi_frame as:
InterpreterFrame gi_frame;
as C won't allow incomplete types in structs, even as the last member.
We could improve this by breaking up the header, so the public API sees a different definition. Still a lie, but a more elegant one.
Public header:
typedef struct { _PyGenObject_HEAD(gi) } PyGenObject;
Private header:
typedef struct { _PyGenObject_HEAD(gi) InterpreterFrame gi_frame; } PyGenObject;
Probably best done in a different PR, though.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Gotcha. Any type other than PyObject* would be better though :-). And if you replace the casts with a macro it’s easier to fix later. I have an idea for the macro but it’s too painful to type on a phone.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
The first field of InterpreterFrame is PyFunctionObject * and the first few fields are all PyObject *, so declaring gi_frame as PyObject * gi_frame[1] seemed safe. Do let me know what your macro is, once you at a PC.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Do let me know what your macro is, once you at a PC.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
PS. What do you mean by "coroutines are stackful"? (I was hoping that reading the code would clarify this, but either I didn't get to that point in the code yet or it's hidden by details.) |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Stackful coroutines have their own stack, like Lua coroutines or greenlets. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
e31703c to
17b453a
Compare
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.