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

AST formatter/pretty-printer #1003

Merged
merged 1 commit into from
Jul 19, 2023
Merged

AST formatter/pretty-printer #1003

merged 1 commit into from
Jul 19, 2023

Conversation

jjcnn
Copy link
Contributor

@jjcnn jjcnn commented Jul 11, 2023

Describe your changes

A default formatter is added to ProgramAst and ModuleAst. The formatter acts as a pretty-printer for the AST, including indentation and procedure names rather than procedure indices/ids.

In order to perform indentation and to output legal assembly, a set of Formattable structs have been added. These keep track of the formatting context.

Call instructions are handled separately from other instructions, because the procedure indices and ids must be transformed into procedure names.

No tests have been added at this stage, because the formatter will be tested when connected to the compiler. It might also be worthwhile to add a -pp or -format option to the command-line assembler - comments regarding this feature would be appreciated.

No documentation has been added, since this PR is not user-facing.

Checklist before requesting a review

  • Repo forked and branch created from next according to naming convention.
  • Commit messages and codestyle follow conventions.
  • Relevant issues are linked in the PR description.
  • Tests added for new functionality.
  • Documentation/comments updated according to changes.

@jjcnn jjcnn force-pushed the jjcnn-ast-pretty-printer branch 3 times, most recently from fdb5ea7 to 93cc387 Compare July 11, 2023 14:29
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! I left a few small comments inline.

assembly/src/ast/imports.rs Show resolved Hide resolved
assembly/src/ast/mod.rs Outdated Show resolved Hide resolved
assembly/src/ast/mod.rs Outdated Show resolved Hide resolved
Comment on lines 332 to 334
/// # Panics
///
/// Import info must be added before the program can be displayed.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Let's add a brief description (above the # Panics line) to say that this writs fully formatted MASM with correct indentation etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's update the comment to:

Writes this [ProgramAst] as formatted MASM code into the formatter.

The formatted code puts each instruction on a separate line and preserves correct indentation
for instruction blocks.

# Panics
Panics if import info is not associated with this program.

assembly/src/ast/mod.rs Outdated Show resolved Hide resolved
assembly/src/ast/mod.rs Outdated Show resolved Hide resolved
assembly/src/ast/nodes/format.rs Outdated Show resolved Hide resolved
@bobbinth
Copy link
Contributor

It might also be worthwhile to add a -pp or -format option to the command-line assembler.

Which command would this be relevant for? Or are you thinking about adding a new command - e.g., miden format? If the latter, I think this can be left for a future PR.

@jjcnn
Copy link
Contributor Author

jjcnn commented Jul 18, 2023

It might also be worthwhile to add a -pp or -format option to the command-line assembler.

Which command would this be relevant for? Or are you thinking about adding a new command - e.g., miden format? If the latter, I think this can be left for a future PR.

My plan was to add it as an option to an existing command, e.g., compile: miden compile -pp file.masm -o formatted.masm.

We could add it as a new command instead, if you think that's better.

I'm happy to leave that to a future PR.

@jjcnn
Copy link
Contributor Author

jjcnn commented Jul 18, 2023

Review comments addressed.

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! I left a few more comments inline. Once these are addressed, we can merge.

assembly/src/ast/imports.rs Show resolved Hide resolved
assembly/src/ast/nodes/format.rs Outdated Show resolved Hide resolved
Comment on lines 332 to 334
/// # Panics
///
/// Import info must be added before the program can be displayed.
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's update the comment to:

Writes this [ProgramAst] as formatted MASM code into the formatter.

The formatted code puts each instruction on a separate line and preserves correct indentation
for instruction blocks.

# Panics
Panics if import info is not associated with this program.

assembly/src/ast/mod.rs Outdated Show resolved Hide resolved
@bobbinth
Copy link
Contributor

We could add it as a new command instead, if you think that's better.

I'm happy to leave that to a future PR.

Yes, let's hold off on this for now. One of the reasons is that we'd erase all internal comments from the input file - and I'm not sure if that's a good idea.

@jjcnn
Copy link
Contributor Author

jjcnn commented Jul 19, 2023

Review comments addressed, and PR rebased.

@bobbinth bobbinth merged commit 0bc80b0 into next Jul 19, 2023
15 checks passed
@bobbinth bobbinth deleted the jjcnn-ast-pretty-printer branch July 19, 2023 20:30
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.

2 participants