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

Ignore function call casing, persist parameter % #175

Conversation

MarquessV
Copy link
Contributor

@MarquessV MarquessV commented Mar 21, 2023

There were two issues to solve here:

  1. Ignore casing for function calls

This may be controversial. The Quil spec only ever refers to cis in a lowercase form. I've created this issue for clarification in the spec.

  1. Fix parameter output

Parameters for GateDefintion and WaveDefinition are stored in a Vec<String>. These parameters are Quil identifiers prefixed with a % (aka "formal parameters). The parser recognizes these as formal parameters, but stores them on the instruction without the %. This meant they were getting printed back out as Quil without the %, which is incorrect. This fix restores the % on output, though it may make more sense to keep it in the parser. After further discussion we've decided to omit the % as it is a delimiter for quil and doesn't need to be part of the internal representation of a parameter. I've added tests to confirm this approach works for GateDefinition, MeasureCalibrationDefinition and WaveformDefinition.

@MarquessV MarquessV changed the title 174 fix expression defgate parsing Ignore function call casing, persist parameter % Mar 21, 2023
@MarquessV MarquessV marked this pull request as ready for review March 22, 2023 23:04
Copy link
Contributor

@Shadow53 Shadow53 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious about the use of r"" over "" in a couple places, otherwise this looks fine to me.

quil-rs/src/instruction/mod.rs Outdated Show resolved Hide resolved
quil-rs/src/instruction/mod.rs Outdated Show resolved Hide resolved
quil-rs/src/instruction/mod.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@kalzoo kalzoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm 👍

quil-rs/src/instruction/mod.rs Outdated Show resolved Hide resolved
@MarquessV MarquessV merged commit a3b0334 into 1455-python-support-for-program-and-beyond Mar 23, 2023
@MarquessV MarquessV linked an issue Apr 10, 2023 that may be closed by this pull request
@MarquessV MarquessV mentioned this pull request May 25, 2023
5 tasks
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.

Recognize expression function calls regardless of casing
3 participants