| msg203845 - (view) |
Author: Larry Hastings (larry) *  |
Date: 2013-11-22 19:42 |
| Attached is a patch exposing the old opcode_stack_effect() function to Python. The patch does the following: * renames opcode_stack_effect() to PyCompile_OpcodeStackEffect() * removes the "static" modifier from PyCompile_OpcodeStackEffect() * changes PyCompile_OpcodeStackEffect()'s behavior so it returns a magic value on failure * preserves existing behavior when compiling code and encountering an opcode/oparg pair that results in failure * creates a new _opcode module * exposes PyCompile_OpcodeStackEffect() as _opcode.stack_effect() * tests _opcode module with new test__opcode.py * imports _opcode.stack_effect() into opcode, exposing it publically * documents the function in dis (there is no documentation for opcode, and dis imports and exposes everything in opcode) Whew! I think it's ready to go in. |
|
|
| msg203847 - (view) |
Author: Larry Hastings (larry) * |
Date: 2013-11-22 19:43 |
| (Sponging around for a reviewer ;-) |
|
|
| msg203856 - (view) |
Author: Eric Snow (eric.snow) * |
Date: 2013-11-22 20:15 |
| FWIW, I agree with Antoine about making those PyCompile_ functions private (leading "_"). |
|
|
| msg203857 - (view) |
Author: Larry Hastings (larry) * |
Date: 2013-11-22 20:24 |
| New patch, incorporating Antoine's comments. Thanks, Antoine! |
|
|
| msg203922 - (view) |
Author: Alyssa Coghlan (ncoghlan) * |
Date: 2013-11-22 23:42 |
| +1 from me. A stack_effect attribute on dis.Instruction would be a nice bonus, but doesn't need to be part of this patch. |
|
|
| msg203931 - (view) |
Author: Alyssa Coghlan (ncoghlan) * |
Date: 2013-11-23 00:37 |
| Hmm, looking at dis.py, I'm -1 on exposing this as a public opcode module API at this point, although I'm still a fan of exposing it as opcode._stack_effect to allow advanced users access (ala sys._get_frames). I initially thought the required addition to dis.Instruction would just be: @property def stack_effect(self): return opcode.stack_effect(self.opcode, self.arg) However, that doesn't necessarily work, since self.arg may be None. That means stack_effect has to be at least: def stack_effect(opcode, oparg=None): if oparg is None: if opcode >= HAVE_ARGUMENT: raise ValueError("This opcode needs an argument") oparg = 0 return _opcode.stack_effect(opcode, oparg) However, even that's not quite accurate, since if the previous opcode was EXTENDED_ARG, you should be adding *that* arg times 65536 to oparg in order to figure out the stack effect. It's that need to take the previous opcode into account to correctly determine the value for "oparg" that makes this a bit tricky. Although, I guess the latter concern would only apply to integration into the dis module - for the opcode module, it just needs to be documented that the calculation of the passed in oparg value should take EXTENDED_ARG into account. |
|
|
| msg203937 - (view) |
Author: Larry Hastings (larry) * |
Date: 2013-11-23 00:51 |
| stack_effect will never know about EXTENDED_ARG. Instead, you simply pass the already-extended arg as the second argument. Making the second argument support a default of None will be slightly inconvenient, but I'll do it if you think it's a big improvement. Considering how troublesome it was to recreate this information when I wrote my assembler, I definitely think this information should be exported out of the murky heart of Python. |
|
|
| msg204028 - (view) |
Author: Alyssa Coghlan (ncoghlan) * |
Date: 2013-11-23 14:23 |
| Yeah, I argued myself into realising EXTENDED_ARG just needed to be mentioned in the function docs, but didn't go back and fix my opening paragraph. The fact dis uses an arg of None (rather than zero) to indicate "no argument" means I think the extra layer of wrapping in the pure Python module is needed prior to 3.4 rc1, but we can live without it for beta 1. |
|
|
| msg204113 - (view) |
Author: Larry Hastings (larry) * |
Date: 2013-11-23 22:06 |
| Attached is revision 3 of the patch. I'm gonna check it in pretty soon, so as to make beta (and feature freeze). I changed the API so the oparg is optional, and it raises if it gets one it shouldn't have or didn't get one when it should. |
|
|
| msg204122 - (view) |
Author: Roundup Robot (python-dev)  |
Date: 2013-11-23 22:50 |
| New changeset 5fe72b9ed48e by Larry Hastings in branch 'default': Issue #19722: Added opcode.stack_effect(), which accurately http://hg.python.org/cpython/rev/5fe72b9ed48e |
|
|
| msg213006 - (view) |
Author: Roundup Robot (python-dev) |
Date: 2014-03-10 01:35 |
| New changeset 4a801f8b7e2d by R David Murray in branch 'default': whatsnew: dis.stack_effect (#19722). http://hg.python.org/cpython/rev/4a801f8b7e2d |
|
|