Skip to content
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

Consistent whitespace before duration units and imaginary literal. #496

Conversation

hodgestar
Copy link
Contributor

Summary

This PR makes two small adjustments to whitespace lexing, one for duration units and the other for imaginary literals.

Duration units whitespace

For duration units, the language specification currently reads:

Units can appear attached to the numerical value, or immediately following separated only by blanks or tabs. 1000ms is the same as 1000 ms.

But this whitespace was not actually supported by the lexer.

This PR adds this support to match the specification.

Imaginary literal whitespace

For imaginary literals, the language specification currently reads:

Imaginary literals are written as a decimal-integer or floating-point literal followed by the letters im. There may be zero or more spaces between the numeric component and the im component.

This is correctly implemented, but it seems inconsistent to only allows spaces here and not tabs. Tabs and spaces are both allowed for units and generally wherever whitespace is allowed.

This PR updates the specification and the lexer to allow tabs in this case too.

Details and comments

The changes to the lexer appear straightforward. Adding tabs to the YAML reference files required the unfortunate use of literal tab characters, but the other options (double quoted YAML blocks) looked worse.

@braised-babbage
Copy link
Contributor

braised-babbage commented Dec 4, 2023

Thank you for catching this. Your changes look correct to me.

I guess the alternative would have been to make the spec more restrictive. I do wonder about the merits of allowing whitespace within tokens. If there was a linter or style guide it would probably force a standard so that programs don't mix 10ns and 10 ns and 10 ns; and in my opinion the former is slightly easier for a reader to interpret. Similarly with languages that have modifiers for float literals, where 1e-3 is fine but 1 e -3 is invalid (even though it could in principle be supported).

@hodgestar
Copy link
Contributor Author

@braised-babbage I'm happy to make a new PR if the decision is taken to tighten the spec instead. For now this just updates the code to match the spec.

I personally find 10 ns a bit more readable than 10ns and 1.517e3 ns a lot more readable than 1.517e3ns, but one could definitely argue this a few ways. The no-spaces-in-float-literals rule for me is kind of an argument for allowing space here -- it's a number and a unit, so it can be viewed as two things.

@hodgestar
Copy link
Contributor Author

@braised-babbage And thanks for reviewing!

@braised-babbage
Copy link
Contributor

@braised-babbage I'm happy to make a new PR if the decision is taken to tighten the spec instead. For now this just updates the code to match the spec.

I personally find 10 ns a bit more readable than 10ns and 1.517e3 ns a lot more readable than 1.517e3ns, but one could definitely argue this a few ways. The no-spaces-in-float-literals rule for me is kind of an argument for allowing space here -- it's a number and a unit, so it can be viewed as two things.

I'm inclined to agree with you, particularly in the case of 1.517e3 ns.

@levbishop levbishop merged commit 909379e into openqasm:main Dec 20, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants