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

chore: separate methods in dc_crate into their own modules #3585

Merged
merged 5 commits into from
Nov 28, 2023

Conversation

kevaundray
Copy link
Contributor

Description

This is a first try at modularizing the functionality in dc_crate so that we can document and if the right abstraction is applied, test these components nicely.

The traits implementation is quite big, so isolating that large code chunk is also an advantage.

Problem*

Resolves

Summary*

Additional Context

Documentation*

Check one:

  • No documentation needed.
  • Documentation included in this PR.
  • [Exceptional Case] Documentation to be submitted in a separate PR.

PR Checklist*

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt on default settings.

@kevaundray kevaundray marked this pull request as ready for review November 26, 2023 23:37
Copy link
Contributor

@jfecher jfecher left a comment

Choose a reason for hiding this comment

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

Not sure how I feel about this change. I feel like splitting all these functions into different modules like this is probably a symptom of the bigger problem that the name resolution pass is too messy in general.

I do appreciate these being in a "resolution" folder rather than a def_collector folder. We should try to split the two parts of this pass more in the future. The split isn't exact though since there are still some functions like collect_impls in the resolution folder. I'd need to review whether this should be in the def_collector folder instead though since there are cases where some constructs have 3 steps (collect, collect, resolve or collect, resolve, resolve) instead of the normal 2. I think most of the def collecting / name resolution pass could use cleaning up but that'd go beyond reorganization.

@kevaundray
Copy link
Contributor Author

Not sure how I feel about this change. I feel like splitting all these functions into different modules like this is probably a symptom of the bigger problem that the name resolution pass is too messy in general.

I do appreciate these being in a "resolution" folder rather than a def_collector folder. We should try to split the two parts of this pass more in the future. The split isn't exact though since there are still some functions like collect_impls in the resolution folder. I'd need to review whether this should be in the def_collector folder instead though since there are cases where some constructs have 3 steps (collect, collect, resolve or collect, resolve, resolve) instead of the normal 2. I think most of the def collecting / name resolution pass could use cleaning up but that'd go beyond reorganization.

Yeah I agree with this -- there is a larger issue with the resolution pass. I think to fix it will require multiple PRs that we can flesh out in an issue. I think the re-org is a net positive as it isolates code in such a way that atleast makes it easier to read how a particular language feature is handled and what functions are depending on the resolution of the feature.

@kevaundray kevaundray enabled auto-merge November 28, 2023 17:56
@kevaundray kevaundray added this pull request to the merge queue Nov 28, 2023
Merged via the queue into master with commit 1b5db48 Nov 28, 2023
33 checks passed
@kevaundray kevaundray deleted the kw/try-move-all-resolve-methods-into-modules branch November 28, 2023 18:17
TomAFrench added a commit that referenced this pull request Nov 30, 2023
* master: (216 commits)
  chore(docs): address visibility issues in docs (#3643)
  chore: type formatting (#3618)
  fix: Restrict fill_internal_slices pass to acir functions (#3634)
  chore(docs): docs for v0.19.4 (#3601)
  feat: aztec-packages (#3626)
  chore: Move tests to the correct root (#3633)
  feat: Implement integer printing (#3577)
  fix: corrected the formatting of error message parameters in index out of bounds error (#3630)
  chore: Update ACIR artifacts (#3619)
  chore(debugger): Run debugger REPL in thread (#3611)
  chore: remove deprecated method (#3625)
  feat: Implement raw string literals (#3556)
  fix: docker builds (#3620)
  feat: Copy on write optimization for brillig (#3522)
  chore: separate methods in `dc_crate` into their own modules (#3585)
  feat: add special case for boolean AND in acir-gen (#3615)
  chore: Update ACIR artifacts (#3614)
  chore: remove `get_number_sequence` foreign calls (#3613)
  feat: Add package version to Nargo.toml metadata (#3427)
  chore(docs): Links to Aztec docs from errors (#3423)
  ...
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.

3 participants