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

Provide stdlib via module provider to assembler #450

Merged
merged 3 commits into from
Oct 28, 2022

Conversation

vlopes11
Copy link
Contributor

@vlopes11 vlopes11 commented Oct 26, 2022

Describe your changes

This initial step for the solution of #298 removes stdlibrary as dependency of assembler.

This decoupling will increase the assembler module extraction flexibility. As next step, an efficient lookup of hasher module+label will be introduced in order to fetch parsed/compiled procedures.

@vlopes11 vlopes11 self-assigned this Oct 26, 2022
@vlopes11 vlopes11 added enhancement New feature or request assembly Related to Miden assembly labels Oct 26, 2022
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.

Thank you! Looks good! I left a couple small comments inline.

One more general comment (could be addresses in the future): I don't love making Assembler generic. However, we do want to have the flexibility of defining custom logic for ModuleProvider.

I'm wondering if there is a way to achieve both somehow. For example, could we provide ModuleProvider as dyn reference? (I have very little experience working with such patterns).

Another potential option is making ModuleProvider a struct rather than a trait. This struct then could be built in a variety of ways by whoever needs to instantiate the assembler. This will make the structure less flexible we'll need to make assumptions about how ModuleProvider works.

Are there any other approaches?

miden/src/examples/fibonacci.rs Outdated Show resolved Hide resolved
assembly/src/tests.rs Outdated Show resolved Hide resolved
@vlopes11
Copy link
Contributor Author

@bobbinth there is a nice possibility to have a module provider as concrete that will have runtime loaded modules via dyn ModuleProviderInstance. These can be many, and this will offer further flexibility. Preparing a push with that approach

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.

I like this approach! Thank you! I left a few comments inline (some nits and a question about potential simplification).

assembly/src/lib.rs Show resolved Hide resolved
assembly/src/lib.rs Outdated Show resolved Hide resolved
assembly/src/lib.rs Outdated Show resolved Hide resolved
assembly/src/lib.rs Outdated Show resolved Hide resolved
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.

I like it! Thank you! I left just two small comments inline.

assembly/src/lib.rs Outdated Show resolved Hide resolved
assembly/src/lib.rs Show resolved Hide resolved
@bobbinth bobbinth changed the title Refactor stdlib in assembler Provide stdlib via module provider to assembler Oct 28, 2022
@bobbinth bobbinth mentioned this pull request Oct 28, 2022
26 tasks
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!

@bobbinth bobbinth merged commit 1277e5b into next Oct 28, 2022
@bobbinth bobbinth deleted the refactor-stdlib-in-assembler branch October 28, 2022 21:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assembly Related to Miden assembly enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants