New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
simple patch, improving unreachable bytecode removing #45735
Comments
This patch improves bytecode output, by removing unreachable code. It |
I am not sure what this patch would accomplish. I tried Both with and without patch I get $ ./python.exe -O t.py
2 0 LOAD_CONST 1 (1)
3 RETURN_VALUE 3 4 LOAD_CONST 1 (1) I am sure I am missing something, but it is hard to tell what without |
It would be great to see test cases with this change. That would help |
I figured that out: >>> def g():
... raise RuntimeError
... Before the patch: >>> dis(g)
2 0 LOAD_GLOBAL 0 (RuntimeError)
3 RAISE_VARARGS 1
6 LOAD_CONST 0 (None)
9 RETURN_VALUE
After the patch:
>>> dis(g)
2 0 LOAD_GLOBAL 0 (RuntimeError)
3 RAISE_VARARGS 1 Looks reasonable to me. Paul, do you need help with unit tests? |
Yes, help with unit tests would be appreciated. Especially since it is BTW, trying to understand why the patch didn't remove unreachable code def f1():
return 1
x() disassembly: 4 4 LOAD_GLOBAL 0 (x) def f2():
return 1
x()
return 2 disassembly: Looking a bit further, I noticed this: So, the patch really can't help with your first example, because peephol |
Speaking of which, I propose that codestr[] array is made one byte I mean, it would cost almost nothing, yet will prevent making some def foo(x):
if x >= 0:
return x * 2
raise ValueError |
|
On Mon, Feb 25, 2008 at 1:32 PM, Benjamin Peterson
I presume you meant "are *not* just for bugfixes". |
|
Attached patch adds test_elim_unreachable() unit test. Last two I am still trying to convince myself that the transformation are
I don't think that will be correct. Did you consider the following
allows |
Thanks for writing the test. Yes, I did read the comment. As I understood it, RETURN_VALUE is needed Such proposed fake RETURN_VALUE _must_ be unreachable, so it must not be |
Paul, You are right. I misunderstood that comment myself. I've tried the $ cat t.py
def f():
return 1
1+2
from dis import dis
dis(f)
$ ./python.exe t.py
6 0 LOAD_CONST 0 (None)
3 RETURN_VALUE |
Can you add more tests? It seems that almost all the examples given in while 1:
if cond: return 1
return 2
return 3 There are a bunch more cases that could be added for where the code The #if 0 code should be removed in the patch. Also, since This patch looks good to me, but I'll let Raymond decide. Did all the |
I have added more tests as Neal suggested. See unreachable-code-with- I've removed #if 0 block and changed "Verify" to "Ensure" in the |
Well, I cannot guarantee it will _always_ be eliminated, since I too As your unit tests suggest, the patch doesn't always remove unreachable BTW, I think that just removing #if 0'd block is not correct. I propose |
Actually, it is even better. Since you don't increment codelen, that |
Good catch!
In this case it will be clearer to use STOP_CODE instead of RETURN_VALUE |
Recommend rejecting. Too much work for zero performance payoff. |
Rejecting. Too much risk as well, based on experience in the past with |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: