bpo-45168: change dis output to omit missing values rather than replacing them by their index #28313
Conversation
Is there no way to just not display the parenthetical comment at all?
Yes, I can make get* functions return a "no value" sentinel that gets ignored later. Though I just noticed that all this is documented, so unless we are willing to change the documented API, even what I did here is no good (we'd need to add another field to Instrcution which can be set to what we want to display): https://docs.python.org/3/library/dis.html#dis.Instruction |
|
I think we can change that API, at the top of that page I see this:
|
(Sorry, I think I said the same thing three times. That's what you get when I make a single pass over the review. :-)
Doc/library/dis.rst
Outdated
|
|
||
| .. data:: argrepr | ||
|
|
||
| human readable description of operation argument | ||
| human readable description of operation argument (if known). |
There was a problem hiding this comment.
This should state what it is if not known -- you cannot rely on the "versionchanged" note for that. I wouldn't leave it unset, but I'd be okay if it was one of: None, "", "<unknown>" or dis.UNKNOWN. BTW, what is it for opcodes that don't have an oparg (like RETURN_VALUE)?
There was a problem hiding this comment.
I was planning to define what it is, but then I need to assert that in a unit test and it's actually not possible to create an Instruction object with an UNKNOWN argval through the public API (unless I construct it directly, but then what am I actually checking?). You can generate them by calling get_instruments get_instructions, but that only works for things that do have co_consts and co_names (you can't give it just the bytes of the code).
There was a problem hiding this comment.
get_instruments --> get_instructions (typing instrument is muscle memory you get from working in a bank. I did it several times today)
There was a problem hiding this comment.
Maybe we don't need to document the "unknown value" case at all here, since it's an internal thing of the dis module?
There was a problem hiding this comment.
BTW, what is it for opcodes that don't have an oparg (like
RETURN_VALUE)?
It's an empty string.
Lib/dis.py
Outdated
| @@ -283,7 +290,7 @@ def _disassemble(self, lineno_width=3, mark_as_current=False, offset_width=4): | |||
| if self.arg is not None: | |||
| fields.append(repr(self.arg).rjust(_OPARG_WIDTH)) | |||
| # Column: Opcode argument details | |||
| if self.argrepr: | |||
| if self.argrepr and self.argval is not UNKNOWN: | |||
There was a problem hiding this comment.
This suggests that if argval is UNKNOWN, we could set argrepr to None or an empty string, rather than testing argval?
There was a problem hiding this comment.
We could set it to empty string. I didn't want to make it None because now it's always a string.
| if get_name is not None: | ||
| argval = get_name(name_index, **extrainfo) | ||
| argrepr = argval | ||
| return argval, argval |
There was a problem hiding this comment.
So here I had expected argval, None or argval, "".
There was a problem hiding this comment.
This is the case where argval can be calculated, and since it's a name it's equal to its repr so we don't do repr(argval) for the second item.
Nice.
|
|
||
| .. data:: argrepr | ||
|
|
||
| human readable description of operation argument (if known). | ||
| human readable description of operation argument (if any), |
There was a problem hiding this comment.
I like how you kept it ambiguous whether "if any" refers to the argument or to the description. :-)
https://bugs.python.org/issue45168
The text was updated successfully, but these errors were encountered: