-
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
Implement Repl tool in Miden VM #440
Conversation
When I implemented the cli I put all cli specific dependencies and logic behind a feature flag - |
Thank you! I tested it a bit - so, these are comments based on usage not a code review: Upon launch, the printout looks a bit odd:
We probably don't need the part "Launching Miden Repl Tool". Then, for some reason only the
All command should be prefixed so that they are not confused with assembly instructions. For Next, when we execute a command, we print out top 16 elements of the stack and stack helper registers.
What's the reason for printing stack helper registers? I think they are pretty meaningless from the user's standpoint. Instead, in place of stack helper registers we should print out values in the stack overflow table. We also probably should get rid of the blank line between the input and stack state printout (i.e., in the example above the blank line between Next, when we print out the state of memory, we print elements in debug mode:
Instead, we should print them out as integers, and generally improve formatting to be more like this:
Next, when printing out individual memory location we currently show something like this:
Instead, we should show something like this:
Lastly, when printing out an empty memory slot, we should something like this:
Instead, it could be something like this:
|
Thanks for the review! I've actioned on your suggestions.
I've updated it to
I've now prefixed all the REPL commands with
Agreed and I've removed the overflow part and the extra line in between the commands and the output.
The output of
Although I feel that we should display the memory values in reverse order as then it will be inline with the order we have in stack. |
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.
Looks good! Thank you! I left some comments inline.
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! I left more small comments inline.
Signed-off-by: 0xKanekiKen <100861945+0xKanekiKen@users.noreply.github.com>
Signed-off-by: 0xKanekiKen <100861945+0xKanekiKen@users.noreply.github.com>
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 PR addresses #423 and is in continuation to the work done by the scribe team here.
The following tasks are addressed in this PR:
Checklist before requesting a review
next
according to naming convention.