-
Notifications
You must be signed in to change notification settings - Fork 158
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
Conversation
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.
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?
@bobbinth there is a nice possibility to have a module provider as concrete that will have runtime loaded modules via |
1674118
to
025b62f
Compare
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.
I like this approach! Thank you! I left a few comments inline (some nits and a question about potential simplification).
025b62f
to
a98593a
Compare
a98593a
to
cb1e804
Compare
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.
I like it! Thank you! I left just two small comments inline.
cb1e804
to
7c2868a
Compare
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.
All looks good! Thank you!
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.