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

Format MASM #1678

Open
wants to merge 3 commits into
base: next
Choose a base branch
from
Open

Conversation

partylikeits1983
Copy link
Contributor

I ran my Miden Assembly code formatter on the stdlib directory and discovered several formatting issues:

  • Trailing white space at the end of lines
  • Inconsistent indentation of comments
  • Inconsistent indentation inside procedures
  • Consecutive empty lines

Note: There are large chunks where there are modifications, this is where indentation is changed to 4 spaces as opposed to two.

You can install the masm formatter from crates.io using:

cargo install masm-formatter

To run the formatter on a specific directory (e.g., crates/miden-lib/asm), execute:

cargo masm-fmt stdlib

This PR applies the formatter's changes, addressing the issues noted above.

Copy link
Contributor Author

@partylikeits1983 partylikeits1983 left a comment

Choose a reason for hiding this comment

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

There are certain areas where comments that are on the same line as an instruction don't line up, but I think this should be addressed in a separate PR.

Comment on lines -537 to +539
push.0.0.0.0
push.0.1.1954.954529 # pushed R2's radix-2^32 form;
# see https://github.com/itzmeanjan/secp256k1/blob/6e5e654823a073add7d62b21ed88e9de9bb06869/field/base_field_consts.py#L31
push.0.0.0.0
push.0.1.1954.954529 # pushed R2's radix-2^32 form;
# see https://github.com/itzmeanjan/secp256k1/blob/6e5e654823a073add7d62b21ed88e9de9bb06869/field/base_field_consts.py#L31
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is one of those edge cases that is hard to fix with the masm formatter.

@partylikeits1983
Copy link
Contributor Author

I know this is a massive PR and might take a bit of time to verify that only formatting was changed. Lmk if there is a different way I should structure this PR.

Also, we can maybe add the MASM formatter to the rust compiler and then add it as a CI check.

@bobbinth bobbinth added the no changelog This PR does not require an entry in the `CHANGELOG.md` file label Mar 4, 2025
@plafer
Copy link
Contributor

plafer commented Mar 4, 2025

Very cool!

Probably best to wait for big MASM PRs like #1665 to be merged before running the formatter though.

Also, we can maybe add the MASM formatter to the rust compiler and then add it as a CI check.

Is it reliable enough to be added as CI check? For example, in your comment, didn't the formatter do the wrong thing?

@partylikeits1983
Copy link
Contributor Author

Is it reliable enough to be added as CI check? For example, in your comment, didn't the formatter do the wrong thing?

I think to be added as a CI check it needs to be tested a bit more, and maybe added to the miden compiler toolchain, or as a separate standalone repo.

Regarding formatting comments that are on the same lines as code and span multiple lines, it depends if it is necessary to support this, or just have the comment on the line above the code. What do you think?

@plafer
Copy link
Contributor

plafer commented Mar 5, 2025

Looking more into it (especially partylikeits1983/masm-formatter#3), it seems like we'd probably want to reuse our internal pretty print infrastructure to make sure the formatter is consistent with what we pretty print. But until that happens, running it every once in a while like you did here sounds like a good alternative!

So I think I would wait for the main MASM-related PRs to be merged, and then run a formatter pass.

Blocked on: #1665, and maybe #1670, #1675

@plafer plafer added the on hold This issue is blocked or we don't want to start it yet label Mar 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no changelog This PR does not require an entry in the `CHANGELOG.md` file on hold This issue is blocked or we don't want to start it yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants