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

Update: parse ternary, binary, and unary logic #58

Merged
merged 13 commits into from
Jun 7, 2022
Merged

Conversation

nilslice
Copy link
Contributor

@nilslice nilslice commented Mar 14, 2022

src/instruction.rs Outdated Show resolved Hide resolved
src/parser/command.rs Outdated Show resolved Hide resolved
@nilslice
Copy link
Contributor Author

Copy link
Contributor

@genos genos left a comment

Choose a reason for hiding this comment

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

A great start! Looking forward to seeing where this goes.

src/instruction.rs Outdated Show resolved Hide resolved
src/instruction.rs Outdated Show resolved Hide resolved
src/parser/lexer.rs Outdated Show resolved Hide resolved
src/parser/command.rs Show resolved Hide resolved
src/program/memory.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@notmgsk notmgsk left a comment

Choose a reason for hiding this comment

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

Aside from the clean up needed (commented code, TODOs) and removing the deprecated instructions, LGTM!

I'll defer approval until it's no longer marked draft.

src/instruction.rs Outdated Show resolved Hide resolved
@nilslice nilslice changed the title feat: parse binary logic Feature: parse binary and unary logic Mar 14, 2022
@nilslice
Copy link
Contributor Author

Also added unary logic parsing, but omitted the deprecated TRUE and FALSE operations. Will update this PR accordingly once the final decision is made re: deprecated OR binary operation.

@nilslice nilslice marked this pull request as ready for review March 14, 2022 21:32
@nilslice nilslice changed the title Feature: parse binary and unary logic Update: parse binary and unary logic Mar 14, 2022
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.

Small Q / fix - but otherwise lgtm 👍

src/program/memory.rs Show resolved Hide resolved
src/program/memory.rs Show resolved Hide resolved
@nilslice nilslice changed the title Update: parse binary and unary logic Update: parse ternary, binary, and unary logic Mar 15, 2022
@nilslice
Copy link
Contributor Author

@notmgsk @kalzoo - I've merged #60 into this branch, which adds support for ternary logic operations. Would appreciate a review when you have a moment. If you want to look at only the new ternary support, you can see a clean diff in #60.

@nilslice
Copy link
Contributor Author

Another note, more generally speaking, open to renaming things like TernaryOperator to ComparisonOperator or similar. The grouping of these follows the Quil spec, but the naming is a bit more broad. For example, a LOAD is also ternary but is separated out on its own.

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.

Aside from the possible naming change (and a fmt), looks great!

src/instruction.rs Show resolved Hide resolved
src/instruction.rs Outdated Show resolved Hide resolved
src/instruction.rs Show resolved Hide resolved
@genos
Copy link
Contributor

genos commented Jun 3, 2022

LGTM! 👍

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.

Looks good, thanks!

src/parser/instruction.rs Outdated Show resolved Hide resolved
@nilslice nilslice merged commit 4fb17a9 into main Jun 7, 2022
@nilslice nilslice deleted the sm/instructions branch June 7, 2022 20:20
@github-actions
Copy link

github-actions bot commented Jun 7, 2022

🎉 This PR is included in version 0.10.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants