◐ Shell
reader mode source ↗
Skip to content

bpo-24340: Fix estimation of the code stack size.#5076

Merged
serhiy-storchaka merged 10 commits into
python:masterfrom
serhiy-storchaka:stack-size
Jan 9, 2018
Merged

bpo-24340: Fix estimation of the code stack size.#5076
serhiy-storchaka merged 10 commits into
python:masterfrom
serhiy-storchaka:stack-size

Conversation

@serhiy-storchaka

@serhiy-storchaka serhiy-storchaka commented Jan 1, 2018

Copy link
Copy Markdown
Member

Tests based on patch by Antoine Pitrou (GH #2827).

https://bugs.python.org/issue24340

@serhiy-storchaka serhiy-storchaka added the type-bug An unexpected behavior, bug, or error label Jan 1, 2018
@serhiy-storchaka serhiy-storchaka requested review from a team and pitrou January 1, 2018 20:06
serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this pull request Jan 1, 2018
@pitrou

pitrou commented Jan 1, 2018

Copy link
Copy Markdown
Member

I'd rather see https://bugs.python.org/issue17611 finished than review another bytecode PR.

@serhiy-storchaka

Copy link
Copy Markdown
Member Author

I prefer to fix separate issues by separate commits. Fixing this issue will make patches for bpo-17611 simpler. Currently they contain changes for fixing this issue (not completely).

@serhiy-storchaka

Copy link
Copy Markdown
Member Author

In particularly this could limit changes in PyCompile_OpcodeStackEffect by only changes related to new and changed opcodes.

@pitrou

pitrou commented Jan 1, 2018

Copy link
Copy Markdown
Member

OTOH reviewing two bytecode and compiler PRs is more work than reviewing a single one. And I would like to see how Mark's approach works with this.

@serhiy-storchaka

Copy link
Copy Markdown
Member Author

Isn't reviewing reviewing two separate PRs for two issues easier than reviewing the single PR that mixes fixes of two issues in the same code? Or rather reviewing three PRs every of them fixing the same two issues by slightly different code?

@pitrou

pitrou commented Jan 2, 2018

Copy link
Copy Markdown
Member

Well, those three PRs will have to be regenerated because of conflicts after this PR gets merged.

In any case, if you want to go forward with this, I would like to see more comments (especially on opcodes with a variable stack effect).

@serhiy-storchaka

Copy link
Copy Markdown
Member Author

In this PR I use slightly different approach than in #5006, and it looks more correct to me. Now I better understand this code. I'll update #5006 accordingly.

@serhiy-storchaka

Copy link
Copy Markdown
Member Author

What information do you want be added?

@brettcannon brettcannon removed the request for review from a team January 3, 2018 19:31
@serhiy-storchaka

Copy link
Copy Markdown
Member Author

Could you please take a look again @pitrou? All stack effects are now defined in a single place.

@pitrou

pitrou commented Jan 9, 2018

Copy link
Copy Markdown
Member

This looks ok to me. I can't guarantee that the compiler changes for async for and try...except are right, but the test suite validates them.

@serhiy-storchaka serhiy-storchaka merged commit d4864c6 into python:master Jan 9, 2018
@serhiy-storchaka serhiy-storchaka deleted the stack-size branch January 9, 2018 19:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type-bug An unexpected behavior, bug, or error

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants