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

Documentation fixes #1506

Merged
merged 2 commits into from
Sep 23, 2024
Merged

Documentation fixes #1506

merged 2 commits into from
Sep 23, 2024

Conversation

PhilippGackstatter
Copy link
Contributor

@PhilippGackstatter PhilippGackstatter commented Sep 23, 2024

Describe your changes

A small number of fixes for the VM documentation.

  • The number of locals a procedure may have and the total number of locals for all procedures was documented in two places and both numbers were different:

"The number of locals per procedure is not limited, but the total number of locals available to all procedures at runtime must be smaller than 2^32." (https://0xpolygonmiden.github.io/miden-vm/user_docs/assembly/io_operations.html#random-access-memory)
"A procedure can have at most 2^16 locals, and the total number of locals available to all procedures at runtime is limited to 2^30." (https://0xpolygonmiden.github.io/miden-vm/user_docs/assembly/code_organization.html#procedures). (This should be the correct one afaict).

  • stack and advstack was used for the advice stack, so I made it consistently use advstack since stack is used for the operand stack.
  • Fix rendering of list.
  • Fix a typo.

I assume this PR could use the no changelog label since nothing new is introduced?

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.

Copy link
Contributor

@plafer plafer left a comment

Choose a reason for hiding this comment

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

LGTM! (although I didn't verify the statement about $2^{16}$ locals, but optimistically assuming it's correct)

@PhilippGackstatter
Copy link
Contributor Author

PhilippGackstatter commented Sep 23, 2024

LGTM! (although I didn't verify the statement about 2 16 locals, but optimistically assuming it's correct)

Thank you!

At least given that the num_locals is a u16 this should be correct, as it happens to match exactly the data type.

pub struct Procedure {
/// The source span of the full procedure body
span: SourceSpan,
/// The documentation attached to this procedure
docs: Option<Span<String>>,
/// The local name of this procedure
name: ProcedureName,
/// The visibility of this procedure (i.e. whether it is exported or not)
visibility: Visibility,
/// The number of locals to allocate for this procedure
num_locals: u16,

Edit: Actually, shouldn't it be 2^(16) - 1 then 🤔?

The playground agrees 😄

Error: Failed to compile program - "Failed to compile program - number of procedure locals cannot be greater than 65535 characters, but was 65536"

@plafer plafer added the no changelog This PR does not require an entry in the `CHANGELOG.md` file label Sep 23, 2024
@plafer plafer merged commit 097f76d into next Sep 23, 2024
9 of 10 checks passed
@plafer plafer deleted the pgackst-doc-fixes branch September 23, 2024 16:42
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants