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

Enable function invocation by name #30

Merged
merged 2 commits into from
Jul 17, 2024

Conversation

george-cosma
Copy link
Collaborator

Pull Request Overview

This pull request adds the ability to invoke a function using its name rather than its id.

This feature is needed as part of parsing wast files. Their equivalent of assert uses the name of functions. Relevant issue: #9

Testing Strategy

This pull request was tested by modifying the add_one integration test to invoke its function by name rather than by index.

TODO or Help Wanted

This pull request still needs...

  • Change all tests to use named invocation (Q: Do we want to do this, or let the old tests as-is?)
  • Q: Should RuntimeInstance be responsible for holding onto the exports?

Formatting

  • Ran cargo fmt
  • Ran cargo check
  • Ran cargo build
  • Ran nix fmt
  • Ran treefmt

Author

Signed-off-by: George Cosma [email protected]

@george-cosma
Copy link
Collaborator Author

PR marked as draft until I get some feedback for the question in the "Todo section"

Copy link
Collaborator

@nerodesu017 nerodesu017 left a comment

Choose a reason for hiding this comment

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

lgtm

@valexandru
Copy link
Collaborator

Looks good also to me. I see the value in modifying the other tests so that the code is more readable.
Regarding the question, what does it mean holding onto the exports? Storing the references to them? I think so.

@florianhartung
Copy link
Collaborator

Change all tests to use named invocation (Q: Do we want to do this, or let the old tests as-is?)

Using named invocations in the tests does not really have any downsides and provides better readability, so I'd say it's worth it.


Should RuntimeInstance be responsible for holding onto the exports?

Yes, the RuntimeInstance is our equivalent of the Module Instance as specified by the WASM Spec

@george-cosma george-cosma marked this pull request as ready for review July 12, 2024 13:47
@wucke13
Copy link
Collaborator

wucke13 commented Jul 17, 2024

The changes lgtm, however I would wish for the commit history to be reworked into two commits, one for the initial draft, one for the change of the tests.

@wucke13 wucke13 self-requested a review July 17, 2024 08:40
@george-cosma george-cosma force-pushed the feature/named_invoke branch 2 times, most recently from 92b177c to 3562f6a Compare July 17, 2024 09:22
@wucke13
Copy link
Collaborator

wucke13 commented Jul 17, 2024

@george-cosma please refactor the two commit messages to be compliant with conventional commit. If you rebase from main, you will also get a CI check for that.

@george-cosma george-cosma force-pushed the feature/named_invoke branch from 3562f6a to 383e5cb Compare July 17, 2024 12:36
@george-cosma
Copy link
Collaborator Author

Done 👍

@wucke13 wucke13 added this pull request to the merge queue Jul 17, 2024
Merged via the queue into DLR-FT:main with commit 102143b Jul 17, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants