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.

Sign up for GitHub

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

Jump to bottom

Add support for variables to parser #65

Merged
merged 6 commits into from
Dec 6, 2022
Merged

Add support for variables to parser #65

merged 6 commits into from
Dec 6, 2022

Conversation

tohrnii
Copy link
Contributor

@tohrnii tohrnii commented Nov 5, 2022 •
edited
Loading

Partly addressing #61.

This PR adds support for variables to the parser.

One question here is if there's a better name than TransitionElem. This represents either a declared variable in the transition_constraints section or a transition constraint and similarly for BoundaryElem.

@tohrnii tohrnii changed the base branch from main to next November 13, 2022 19:18
@tohrnii tohrnii changed the title Variable parsing Add support for variables to parser Nov 13, 2022
@tohrnii tohrnii marked this pull request as ready for review November 13, 2022 21:31
@tohrnii tohrnii requested review from grjte and bobbinth and removed request for grjte November 13, 2022 21:31
@tohrnii tohrnii mentioned this pull request Nov 22, 2022
Merged
14 tasks
@grjte grjte removed the request for review from bobbinth November 25, 2022 10:46
@tohrnii tohrnii marked this pull request as draft November 25, 2022 13:59
@tohrnii tohrnii force-pushed the variable_parsing branch 2 times, most recently from 3e4a047 to 7012689 Compare December 3, 2022 22:42
@tohrnii tohrnii force-pushed the variable_parsing branch 3 times, most recently from 210f0cf to e28c908 Compare December 4, 2022 05:17
@tohrnii tohrnii marked this pull request as ready for review December 4, 2022 05:18
bobbinth
bobbinth reviewed Dec 5, 2022
View reviewed changes
Copy link
Contributor

@bobbinth bobbinth 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! Thank you! I left a few comments/questions inline.

parser/src/ast/boundary_constraints.rs Outdated Show resolved Hide resolved
parser/src/ast/transition_constraints.rs Outdated Show resolved Hide resolved
parser/src/ast/boundary_constraints.rs Outdated Show resolved Hide resolved
parser/src/parser/grammar.lalrpop Outdated Show resolved Hide resolved
parser/src/parser/grammar.lalrpop Outdated Show resolved Hide resolved
@tohrnii tohrnii requested a review from bobbinth December 5, 2022 16:39
bobbinth
bobbinth approved these changes Dec 5, 2022
View reviewed changes
Copy link
Contributor

@bobbinth bobbinth 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! Thank you! I left a couple of non-blocking comments inline.

parser/src/parser/tests/arithmetic_ops.rs Outdated Show resolved Hide resolved
parser/src/ast/transition_constraints.rs Outdated Show resolved Hide resolved
parser/src/ast/boundary_constraints.rs Outdated Show resolved Hide resolved
Fumuran
Fumuran reviewed Dec 5, 2022
View reviewed changes
Copy link
Contributor

@Fumuran Fumuran 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!
Just one question — are you planning to implement Variable matching in IR in this PR, or it will be done in the next one?

@tohrnii
Copy link
Contributor Author

tohrnii commented Dec 5, 2022

@Overcastan yes, it will be done in the next PR. I'll open one to address IR and codegen.

bobbinth
bobbinth approved these changes Dec 5, 2022
View reviewed changes
Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

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

All looks good! Thank you!

Al-Kindi-0
Al-Kindi-0 approved these changes Dec 6, 2022
View reviewed changes
Copy link
Collaborator

@Al-Kindi-0 Al-Kindi-0 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 to me, thank you!

@tohrnii tohrnii merged commit ccb5a6a into 0xPolygonMiden:next Dec 6, 2022
@tohrnii tohrnii deleted the variable_parsing branch December 6, 2022 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Al-Kindi-0 Al-Kindi-0 Al-Kindi-0 approved these changes

@bobbinth bobbinth bobbinth approved these changes

@Fumuran Fumuran Fumuran approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tohrnii @Fumuran @bobbinth @Al-Kindi-0