bpo-22352: Adjust widths in the output of dis.dis() for large line…#1153
Conversation
…mbers and instruction offsets. Add tests for widths of opcode names.
|
@serhiy-storchaka, thanks for your PR! By analyzing the history of the files in this pull request, we identified @ncoghlan, @benjaminp and @tim-one to be potential reviewers. |
ncoghlan
left a comment
ncoghlan
left a comment
There was a problem hiding this comment.
Looks like a nice improvement to me.
One inline comment regarding the opcodes that are already more than 20 letters long, but that's really a question for a separate issue.
| def test_widths(self): | ||
| for opcode, opname in enumerate(dis.opname): | ||
| if opname in ('BUILD_MAP_UNPACK_WITH_CALL', | ||
| 'BUILD_TUPLE_UNPACK_WITH_CALL'): |
There was a problem hiding this comment.
While I don't think it should block this change (since it's an existing problem) I wonder if it might it be worth special-casing these for display purposes using something like:
BUILD_MAP_UNPACK_WITH_CALL
...MAP_UNPACK+CALL
12345678901234567890
BUILD_TUPLE_UNPACK_WITH_CALL
...TUPLE_UNPACK+CALL
12345678901234567890
There was a problem hiding this comment.
Sorry, I don't understand your idea. This already is special-cased, the test is skipped for these names.
There was a problem hiding this comment.
I meant in the actual display code, as I assume the columns are currently misaligned for rows containing these two opcodes, whereas they wouldn't be given the abbreviations (...TUPLE_UNPACK+CALL is 20 characters, while ...MAP_UNPACK+CALL is 18).
There was a problem hiding this comment.
I think it is better to misalign columns than output corrupted opcode names.
It may be worth to rename these two opcodes, but this is different issue.
4 participants
… numbers and
instruction offsets.
Add tests for widths of opcode names.