-
Notifications
You must be signed in to change notification settings - Fork 200
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
chore: split up parser into separate files for code organisation #4420
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* master: chore!: reserve `unchecked` keyword (#4432) chore: address code review comments of PR4398 (#4435) feat: Sync from aztec-packages (#4439) feat: Sync from aztec-packages (#4438) chore(docs): correct 'Edit this page' URL for dev docs (#4433) feat: Sync from aztec-packages (#4390) chore(docs): fix external contributor force push workflow (#4437) chore!: Remove empty value from bounded vec (#4431) chore: nargo fmt (#4434) feat: add poseidon2 opcode implementation for acvm/brillig, and Noir (#4398) fix: remove panic when generic array length is not resolvable (#4408) chore(ci): enforce formatting of noir code in CI (#4422) fix: correct formatting for databus visibility types (#4423) chore: remove duplicate `parse_all` function in wasm compiler (#4411) chore(ci): prevent msrv checks from blocking PRs (#4414) feat: expose separate functions to compile programs vs contracts in `noir_wasm` (#4413) chore: do not panic when dividing by zero (#4424)
kevaundray
approved these changes
Feb 28, 2024
jfecher
reviewed
Feb 28, 2024
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.
LGTM
TomAFrench
added a commit
that referenced
this pull request
Mar 2, 2024
* master: (27 commits) feat: skip redundant range checks in brillig (#4460) fix: Variables from trait constraints being permanently bound over when used within a trait impl (#4450) feat: Add overflow and underflow checks for unsigned integers in brillig (#4445) fix(flake): stop flake.nix removing ignored-tests.txt (#4455) fix: build noir_codegen when publishing (#4448) chore: only ignore Nargo.toml in `test_programs` directory and not su… (#4451) chore: create parser specifically for function visibility (#4425) chore: Document BoundedVec (#4430) chore: split up parser into separate files for code organisation (#4420) feat: Sync from aztec-packages (#4444) chore!: reserve `unchecked` keyword (#4432) chore: address code review comments of PR4398 (#4435) feat: Sync from aztec-packages (#4439) feat: Sync from aztec-packages (#4438) chore(docs): correct 'Edit this page' URL for dev docs (#4433) feat: Sync from aztec-packages (#4390) chore(docs): fix external contributor force push workflow (#4437) chore!: Remove empty value from bounded vec (#4431) chore: nargo fmt (#4434) feat: add poseidon2 opcode implementation for acvm/brillig, and Noir (#4398) ...
TomAFrench
added a commit
that referenced
this pull request
Mar 4, 2024
* master: chore: export `report_errors` from `nargo` (#4461) feat: add option to set max memory for bb.js (#4227) feat: backpropagate constants in ACIR during optimization (#3926) feat: skip redundant range checks in brillig (#4460) fix: Variables from trait constraints being permanently bound over when used within a trait impl (#4450) feat: Add overflow and underflow checks for unsigned integers in brillig (#4445) fix(flake): stop flake.nix removing ignored-tests.txt (#4455) fix: build noir_codegen when publishing (#4448) chore: only ignore Nargo.toml in `test_programs` directory and not su… (#4451) chore: create parser specifically for function visibility (#4425) chore: Document BoundedVec (#4430) chore: split up parser into separate files for code organisation (#4420) feat: Sync from aztec-packages (#4444) chore!: reserve `unchecked` keyword (#4432) chore: address code review comments of PR4398 (#4435) feat: Sync from aztec-packages (#4439) feat: Sync from aztec-packages (#4438)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Problem*
Resolves
Summary*
I find the parser quite difficult to navigate due to the fact that it's all in a single file and quite disorganised. For example,
lambda
is 1000 lines belowlambda_parameters
despite it being the only place wherelambda_parameters
is called. This also makes it much harder to see where we have test coverage within the parser due to all the tests being in a single location whereas ideally we'd have these attached to the parser functions they're testing.This PR then splits the parser into submodules based on sections of the grammar which it is responsible for parsing. It's not perfect but is an improvement and we can modify this further if necessary in future.
Additional Context
Documentation*
Check one:
PR Checklist*
cargo fmt
on default settings.