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

Move duplicate procedures to shared util modules #1070

Merged
merged 5 commits into from
Jan 16, 2025

Conversation

PhilippGackstatter
Copy link
Contributor

@PhilippGackstatter PhilippGackstatter commented Jan 16, 2025

After a lot of back and forth in #1002 I think that making a new PR is easier as the approach in this PR is more minimal than in the other PR and reverting those changes in #1002 is more work than applying the necessary changes to next.

This basically implements the conclusion from #1002 (comment)

This is the approach with minimal changes to only deduplicate procedures across kernel and miden lib and reexporting procedures from where they were previously used so the public interface is the same.
The util module was put in a shared directory so we can use Assembler::add_modules_from_dir with namespace kernel to achieve the desired kernel::util::module namespacing. The alternative approach without the shared directory would fail since kernel::util is not a valid LibraryNamespace.

I'm using util instead of utils in anticipation of #1021, even though utils is common and used in std::utils for example. I'm fine with using both.

What we're gaining in code deduplication with this approach we're losing with documentation duplication. We're not yet really doing anything with the MASM docs as far as I'm aware, but when we do, I think it would be great if reexports could inherit the docs from the original definition, unless overwritten.

closes #836

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! Agreed that this is probably the best approach until we have vendored library support. I left just a few small comments/questions inline.

miden-lib/build.rs Show resolved Hide resolved
miden-lib/asm/shared/util/account_id.masm Outdated Show resolved Hide resolved
miden-lib/asm/shared/util/account_id.masm Outdated Show resolved Hide resolved
miden-lib/asm/shared/util/account_id.masm Outdated Show resolved Hide resolved
miden-lib/asm/shared/util/account_id.masm Outdated Show resolved Hide resolved
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 great, thank you @PhilippGackstatter!

I apologize for the inaccurate instructions during our call, but it turned out that I myself did not fully understand what needed to be done. Thank you for the help!

@PhilippGackstatter
Copy link
Contributor Author

I apologize for the inaccurate instructions during our call, but it turned out that I myself did not fully understand what needed to be done. Thank you for the help!

No no, your instructions were great, no worries. I myself was confused about which approach we should go with, so all good!

@PhilippGackstatter PhilippGackstatter merged commit e10fb64 into next Jan 16, 2025
11 checks passed
@PhilippGackstatter PhilippGackstatter deleted the pgackst-remove-duplicate-procedures branch January 16, 2025 16:41
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