bpo-45152: refactor the dis module to make handling of hasconst opcod… #28258
Conversation
iritkatriel
force-pushed
the
bpo-45152a
branch
from
99cd9dc to
16274f8
Compare
…es more generic. Rename constants to co_consts to make it clear which constants this is.
iritkatriel
force-pushed
the
bpo-45152a
branch
from
16274f8 to
64e940a
Compare
| if op == LOAD_CONST: | ||
| if co_consts is not None: | ||
| argval = co_consts[arg] | ||
| return argval |
There was a problem hiding this comment.
I have a patch that exposes the get_const_value function from compile.c in the _opcode module, but I'm on the fence whether it's worth the complication it adds. Let me know if you'd like to see it.
There was a problem hiding this comment.
Why would you do that? Surely it's easier to replicate its functionality in pure Python, even if we add some new ways to represent constants. It doesn't even take a code object currently. And TBH, should e.g. LOAD_NONE even be counted as having a constant? I'd expect HAS_CONST(LOAD_NONE) might well be False.
There was a problem hiding this comment.
I think LOAD_NONE should be HAS_CONST, because the peephole optimiser treats it the same as a LOAD_CONST, for things like folding const tuples and conditional jumps that depend on const.
So far I don't think opcode.hasconst is used for anything (it contains only LOAD_CONST). So I thought we would make it include all the splits of LOAD_CONST, and then HAS_CONST is a C macro generated from that (it's new, I just added it).
There was a problem hiding this comment.
Ah, I see. Currently we take (not the exact notation)
LOAD_CONST None
LOAD_CONST 42
BUILD_TUPLE 2
and transform it to
LOAD_CONST (None, 42)
adding (None, 42) as a new constant to co_consts. We want to still do that even when the first instruction is replaced by LOAD_NONE. That makes sense!
We still have the issue that we might end up with unused constants in co_consts due to this folding. Inada just fixed this for trailing unused constants in GH-28109 (bpo-45056) but for orphaned constants in the middle we have no solution (we'd have to renumber all the LOAD_CONST opargs). Fortunately I think it's not worth it, this is rare.
So you've convinced me that LOAD_NONE should be in opcode.hasconst, but not that it's worth exposing that C function.
There was a problem hiding this comment.
but not that it's worth exposing that C function.
Yeah, I agree it's OTT in this case. We'll just have to implement it twice.
| if op == IMPORT_NAME and i >= 2: | ||
| from_op = opargs[i-1] | ||
| level_op = opargs[i-2] | ||
| if (from_op[0] in hasconst and level_op[0] in hasconst): |
There was a problem hiding this comment.
even this needs hasconst to have LOAD_NONE - the fromlist can be None.
| else: | ||
| return UNKNOWN, '' | ||
| argval = _get_const_value(op, arg, co_consts) | ||
| argrepr = repr(argval) if argval is not UNKNOWN else '' |
There was a problem hiding this comment.
Wondering if repr(UNKNOWN) should just return ''...
There was a problem hiding this comment.
I ruled that out when I thought that the sentinel will be part of the external API, because it could be confusing.
Now it seems more reasonable, because it's only an internal thing, but I still think the sentinel doesn't need to know so much about how it's going to be used/displayed.
…es more generic. Rename constants to co_consts to make it clear which constants this is.
https://bugs.python.org/issue45152