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 — GH-98831: Refactor and fix cases generator by gvanrossum · Pull Request #99526 · python/cpython Conversation
|
|
I have a newer version of the same code in GH-99495 (which adds macros and ops) but that's not fully baked and I figured I'd release this in stages so it's easier on the reviewer. If you'd rather review the tooling in even larger chunks let me know. (I think the changes to the instruction definitions are pretty mild in this one.) |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
brandtbucher
left a comment
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.
Looks good. A few comments:
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
| PyObject *container = SECOND(); | ||
| _PyBinarySubscrCache *cache = (_PyBinarySubscrCache *)next_instr; | ||
| uint32_t type_version = read_u32(cache->type_version); | ||
| inst(BINARY_SUBSCR_GETITEM, (container, sub, unused/1, type_version/2, func_version/1 -- unused)) { |
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.
Super cool to see the cache reads in action!
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
| } | ||
| PyObject *arg = TOP(); | ||
| PyObject *res = _PyCFunction_TrampolineCall(cfunc, PyCFunction_GET_SELF(callable), arg); | ||
| PyObject *res = cfunc(PyCFunction_GET_SELF(callable), arg); |
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.
Did you mean to change this (also the two instances below)?
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
| return self.header.inputs | ||
|
|
||
| @property | ||
| def outputs(self) -> list[StackEffect]: |
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.
| def outputs(self) -> list[StackEffect]: | |
| def outputs(self) -> list[OutputEffect]: |
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
| if ceffect.name != "unused": | ||
| # TODO: if name is 'descr' use PyObject *descr = read_obj(...) | ||
| bits = ceffect.size * 16 |
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.
I had suggestions on these from the other PR, not sure if you saw (I like _ more than unused, and both "unused" and 16 might benefit from constant names in this file).
Unless you just disagreed, in which case, feel free to ignore this. :)
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
| # Write output stack effect assignments | ||
| input_names = [seffect.name for seffect in self.input_effects] | ||
| for i, output in enumerate(reversed(self.output_effects), 1): | ||
| if output.name not in input_names and output.name != "unused": |
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.
For output.name not in input_names, don't we need to check that they occur at the same (stack-adjusted) position, and write them out if not?
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
| while tkn := psr.next(raw=True): | ||
| if tkn.text == BEGIN_MARKER: | ||
| break | ||
| else: | ||
| raise psr.make_syntax_error(f"Unexpected token") | ||
| return instrs, supers, families | ||
| raise psr.make_syntax_error(f"Couldn't find {BEGIN_MARKER!r} in {psr.filename}") |
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.
Wild, I've never seen while/else before...
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
| print( | ||
| f"Unknown instruction {target!r} predicted in {instr.name!r}", | ||
| file=sys.stderr, | ||
| ) | ||
| self.errors += 1 |
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.
You do this move in lots of places... maybe add a method?
def error(self, e: str) -> None: print(e, file=sys.stderr) self.errors += 1
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
| if len(members) < 2: | ||
| continue |
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.
This should be an error, yeah?
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
Reviewers
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assignedLabels
Projects
Milestone
No milestoneDevelopment
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
3 participants
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.