-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Handle final comments with no terminating newline in OpenQASM 2 #10773
Conversation
Previously, if an OpenQASM 2 program to be parsed ended in a comment with no terminating '\n', the lexer would fail to notice the end of the file and would instead attempt to emit the second '/' of the comment introduction as a 'Slash` token.
One or more of the the following people are requested to review this:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM it seems fairly straightforward. Just one question inline about a potential extra test case. If you don't think it's worth it feel free to just add it to the merge queue.
test/python/qasm2/test_structure.py
Outdated
def test_allows_empty(self): | ||
self.assertEqual(qiskit.qasm2.loads(""), QuantumCircuit()) | ||
|
||
@ddt.data("", "\n", "\r\n", "\n ") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you want to test "\n\t"
and "\r\n\t"
too? Just thinking of other whitespace combinations to make the testing here a bit more thorough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extra cases added in c410d75
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only one of these cases that was actually buggy was the one with no whitespace at all, but it doesn't matter a great deal that there's additional cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assumed you were already afk, so I took the liberty of adding them. I hope you dont mind.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's ok, it's all our code anyway.
--------- Co-authored-by: Matthew Treinish <[email protected]>
c410d75
* Handle final comments with no terminating newline in OpenQASM 2 Previously, if an OpenQASM 2 program to be parsed ended in a comment with no terminating '\n', the lexer would fail to notice the end of the file and would instead attempt to emit the second '/' of the comment introduction as a 'Slash` token. * Run `cargo fmt` * Testing extra cases --------- Co-authored-by: Matthew Treinish <[email protected]> --------- Co-authored-by: Luciano Bello <[email protected]> Co-authored-by: Matthew Treinish <[email protected]> (cherry picked from commit 3075f14)
…) (#10778) * Handle final comments with no terminating newline in OpenQASM 2 Previously, if an OpenQASM 2 program to be parsed ended in a comment with no terminating '\n', the lexer would fail to notice the end of the file and would instead attempt to emit the second '/' of the comment introduction as a 'Slash` token. * Run `cargo fmt` * Testing extra cases --------- Co-authored-by: Matthew Treinish <[email protected]> --------- Co-authored-by: Luciano Bello <[email protected]> Co-authored-by: Matthew Treinish <[email protected]> (cherry picked from commit 3075f14) Co-authored-by: Jake Lishman <[email protected]>
Summary
Previously, if an OpenQASM 2 program to be parsed ended in a comment with no terminating '\n', the lexer would fail to notice the end of the file and would instead attempt to emit the second '/' of the comment introduction as a 'Slash` token.
Details and comments
Fix #10770.