Properly fix Jump out of too many nested blocks error
#21
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.
|
Failing test fails due to Python 2.6 no longer existing on test machine, this is not related to my changes. (See Thread) |
There was a problem hiding this comment.
That is brilliant! Thanks for the PR, I am just requesting a few minor changes, if you don't mind.
Also I noticed you added some trailing whitespaces, mind removing those as well?
As for Python 2.6, it's a bummer that it's no longer supported by Travis CI. Neither could I get it to build locally (as it depends on an ancient version of OpenSSL). 😞 So I removed it in 7164121. The tests should pass again after rebasing.
|
Thanks for the reply & the review! a) About python 2.6, that's indeed a shame as I've added some more special cases for it in the other pull requests (I've used a windows binary of 2.6 for testing). b) About merging, if you don't mind - I'll wait for you to review my other two pull requests and if you like them all - I'll edit just the last one according to all of your comments (as it's based on the first two). Then it'll get merged, and the others'll get closed. (Should be less work for both of us this way) |
I'm not sure if I understand correctly, but as I said on the other PR, reviewing the changes in bulk is difficult. So I rather review and land them separately. Also I'd like to squash all commits related to the same feature when landing it rather than having a series of commits with incomplete/partial changes. I'm sorry if this causes you any inconvenience. I very much appreciate your effort! |
|
Okay, I'll update this pull request shortly, then split up the second pull request into two. |
(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)
|
I've added several new commits which: Let me know if any other changes are requested in the refactored code. |
|
Awesome, thanks! I'd like to squash and merge the changes here. However, it seems you used different names / email addresses for different commits. Which one should the squashed commit be attributed to? |
|
(EDIT:) It doesn't matter either way, but you can use the 'condut' one since that's the one I kept using in all the other PRs. |
|
Merged with 399d059. |
|
Thanks! |
This fixes issue #19
The fix is to - if there isn't enough space to write the new bytecode in-place - write it at the end of the bytecode array and add jumps to/from there.
It replaces the previous attempted fix for this issue, as including both would be too complex and
is unneeded.
The tests have been updated (previously they were checking that the issue exists), and a new even more extreme test has been added.
Result observed to work correctly in "real" code. (No, not production code :P)
(I've recreated this PR since the previous PR used the master branch of my fork)