Add implementation of switch to ANTLR and reference AST
#492
Conversation
This adds a simple implementation in both ANTLR and the Python reference AST (and parser) of the `switch` statement propsed by openqasmgh-463.
jakelishman
force-pushed
the
switch-antlr
branch
from
3133134
to
9ba4124
Compare
|
Looks good, We at Atlantic Quantum are looking forward to using this capability in our compiler soon. I was a bit puzzled over not visiting the |
source/openqasm/openqasm3/ast.py
Outdated
| """ | ||
|
|
||
| target: Expression | ||
| cases: Dict[Tuple[int, ...], CompoundStatement] |
There was a problem hiding this comment.
I think we need to allow for actual OpenQASM AST as the keys, rather than just Python int tuples. Unfortunately the Python AST objects are not hashable, so this will need to be a list of pairs.
We won't allow generic expressions in a switch case, but we probably will want to support something more than just integer literals. At the very least, it would be reasonable to allow const expressions (this is akin to C style switches using #define constants, and this is the only way that nontrivial switch statements will be readable). I think this is what is suggested by "integer constant" in the proposal #463. Since a constant is basically anything built from literals or references to const declarations through arithmetic or const externs, my suggestion is that we just allow an arbitrary tuple[Expression, ...] and then it is up to later stages in a compiler or interpreter to check that the switch cases are valid.
There was a problem hiding this comment.
I'd read "integer constant" to mean specifically "integer literal", mostly because there weren't any examples in the text of using constant identifiers, but I think you're right - "constant expression" is the same terminology the C99 spec uses, and 1 + 2 is a valid label in C. It might be worth adding some examples of constant expressions to the spec text to make it clear as well.
A list of pairs is completely fine for the AST, imo. I made it a dict originally only because it was a minor convenience without sacrifice in Python 3.7+, since dict retains insertion order, so it's a proxy for a list. It's no trouble to make it only a list, and I don't think that anything operating on the AST will really care.
Modified in ceac60f. As a side effect, we largely have to lose the duplicate-case detection at this point in AST generation, since we can only reliably do that after constant-propagation, which this generator doesn't do.
There was a problem hiding this comment.
Re: const expressions, this came up at the last TSC meeting and the consensus was that they should get supported. The version of the spec update that got merged mentions it explicitly ("The integer-constant-list-expression is a sequence of one or more integer const expressions separated by comma literals") but I agree that it's worth an example, so I've opened this #499
|
Thanks for the reviews, guys. Erik - your suggestions all sounded good and sensible to me, and hopefully match what you were after now. |
|
@braised-babbage: with this now merged, we probably want to cut a new release of the Python package, but I don't want to get in the way of anything you guys have got going on with it. I'm happy if you want to do it whenever is convenient for you, or if it won't cause problems, I can do it. |
I think it should be fine to make a new release. I will let you have the honor. |
Summary
This adds a simple implementation in both ANTLR and the Python reference AST (and parser) of the
switchstatement proposed by gh-463.Details and comments
This is an implementation of, as I understand things, gh-463 with the addition of "Option 2" from Lev's comment: #463 (comment).
Notably, this only allows one
caseper block with multiple values distinguished by commas, no colon after the values, and the braces are required. I'm happy to update this PR if any of those fairly minor syntactic items change or I misunderstood the intent.