Add builder interface for new switch statement #9919
Conversation
|
Thank you for opening a new pull request. Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient. While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone. One or more of the the following people are requested to review this:
|
Pull Request Test Coverage Report for Build 4704993141
💛 - Coveralls |
With the addition of `switch`, we now have a second control-flow construct that - is not a loop, so can have lazily constructed blocks because of contained `break` and `continue` statements - can have multiple blocks that need their resources unifying For the first part, we largely just have to handle things separately to `if`, though the placeholder instructions are much simpler because the placeholder will never need mutating and replacing (c.f. the `else` block). For the second point, we move a bunch of the previously if-specific code into a shared location in order to reuse it. Unlike the `if` case, the `switch` builder uses nested context managers for its cases, because this mirrors a common indentation pattern for switch statements (at least if you're not a Linux kernel developer). We need new special logic to reject statements that are loose in the `switch` context but not a `case` context. We _could_ have just ignored that, but it feels like an easy potential mistake to make, so much better to loudly fail than silently do the wrong thing.
jakelishman
force-pushed
the
switch/builder
branch
from
924123a to
d0fab50
Compare
Squashed versions of Qiskitgh-9916, Qiskitgh-9919 and Qiskitgh-9926.
There was a problem hiding this comment.
Overall this LGTM, the code seems pretty straightforward and mirroring what is already done for the if/else builder. I had one inline comment on a method I think we should potentially rename. Besides that I think we're missing a few combinations of test cases. There weren't any tests with loops inside a case. Also, I didn't see any test cases mixing while loops and switch statements. I think just for completeness it might be good to validate it all works as expected (especially with break/continue) even though I don't expect anything to break using that because of the existing coverage.
Also I assume the release note for this is integrated into the PR at the top of the stack?
| return _unify_circuit_registers(circuits) | ||
|
|
||
|
|
||
| def _unify_circuit_resources_rebuild( # pylint: disable=invalid-name # (it's too long?!) |
There was a problem hiding this comment.
Lol, it doesn't seem that long to me. Actually, it's not longer than: 6ae6634#diff-6bc399dbeb2cecdb0736e132814faa79639d77be265911ae26d3d9fe32eda3f5R130 which didn't raise an error for me. It might be something else in the regex? Either way this is kind of bogus
There was a problem hiding this comment.
funnily enough, it's because our pylint settings allow longer method names (50 chars) than function names (31 chars)... (Well, that was true of the old .pylintrc form - not certain once we cut out all the duplication.)
|
I wrote the initial release note in #9833, I think with the intention of revisiting it once we were 100% sure of the complete feature set that would make it in. |
* Add builder interface for new switch statement With the addition of `switch`, we now have a second control-flow construct that - is not a loop, so can have lazily constructed blocks because of contained `break` and `continue` statements - can have multiple blocks that need their resources unifying For the first part, we largely just have to handle things separately to `if`, though the placeholder instructions are much simpler because the placeholder will never need mutating and replacing (c.f. the `else` block). For the second point, we move a bunch of the previously if-specific code into a shared location in order to reuse it. Unlike the `if` case, the `switch` builder uses nested context managers for its cases, because this mirrors a common indentation pattern for switch statements (at least if you're not a Linux kernel developer). We need new special logic to reject statements that are loose in the `switch` context but not a `case` context. We _could_ have just ignored that, but it feels like an easy potential mistake to make, so much better to loudly fail than silently do the wrong thing. * Rename inner placeholder_resources calculation * Add missing tests of loops inside switches
Summary
With the addition of
switch, we now have a second control-flow construct thatbreakandcontinuestatementsFor the first part, we largely just have to handle things separately to
if, though the placeholder instructions are much simpler because the placeholder will never need mutating and replacing (c.f. theelseblock). For the second point, we move a bunch of the previously if-specific code into a shared location in order to reuse it.Unlike the
ifcase, theswitchbuilder uses nested context managers for its cases, because this mirrors a common indentation pattern for switch statements (at least if you're not a Linux kernel developer).We need new special logic to reject statements that are loose in the
switchcontext but not acasecontext. We could have just ignored that, but it feels like an easy potential mistake to make, so much better to loudly fail than silently do the wrong thing.Details and comments
As with most of the builder interface PRs, most of the diff is in new tests.