Conversation
(overrides previous fix, as merging them would be too complex and unneeded)
I was not able to get it to happen for pretty huge functions (much much larger than the ones in the test), though, so there's a good chance it's unreachable.
(This was originally in the second PR, but since you want to take this one first, I think it's a good commit to add here, since it's relevant to this change and would make diffing against future PRs easier)
Added 4 tests, 3 currently disabled (pre-existing issues, especially on pypy).
(The new test helps test functionality that needs special attention at least on py38)
|
Details:
|
|
Can you rebase this pull request? (Or alternatively merging |
|
FYI the "Various minor crash/etc fixes" one doesn't fail checks, probably best to skip this PR and merge that. (No point in trying to analyze an already fixed bug) |
| except AttributeError: | ||
| pass | ||
| return code.replace(co_code=codestring) # new in 3.8+ | ||
| except: |
There was a problem hiding this comment.
Please no bare expects, this should be expect AttributeError.
|
|
||
| return size | ||
|
|
||
| def _get_instructions_size(ops): |
There was a problem hiding this comment.
It seems you did a mistake during merge. This function, and the one above are now defined twice, see below.
There was a problem hiding this comment.
Yeah, merge failure. I see you added flake8 which catches this, I'll fix it (in the other PR).
| assert func() == 0 | ||
|
|
||
|
|
||
| def test_jump_out_of_loop_and_survive(): |
There was a problem hiding this comment.
I might (likely) miss something here, but how is this new test related to adding support for Python 3.8?
| self.have_argument = dis.HAVE_ARGUMENT | ||
| self.jump_unit = 1 | ||
|
|
||
| self.has_loop_blocks = 'SETUP_LOOP' in dis.opmap |
There was a problem hiding this comment.
I'm wondering if we should rather check for 'SETUP_LOOP' in dis.opmap in place than aliasing the result of this check here? It doesn't seem to save much writing, neither does it seem to improve performance.
There was a problem hiding this comment.
I think it's cleaner, and it's far from a sole occurrence - see the later PRs which add more such checks, some (I think) used more than once.
|
Yes, let's focus on #22 instead of this PR. It's provided here so you can review it as a "step" separate from #22, but it's evidently missing some fixes included in #22 and there's no reason to start digging which fix is needed. (And I don't see a reason to remove the py38 changes from the #22 PR before merging it, you've already reviewed them here, it'd just be extra busywork) It's already merged against master, and I'll fix it up per your comments and flake8 shortly |
2 participants
Based on PR #21, adds python3.8 support (previously part of the 'fix2' PR)