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

[Feature] Adds importable memory. #42

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Conversation

may-20i
Copy link

@may-20i may-20i commented Oct 11, 2024

What do you think of something like this for memory imports?

@may-20i may-20i changed the title Adds memory imports to macros. [Feature] Adds importable memory. Oct 11, 2024
@RoyalIcing
Copy link
Owner

Interesting! I like it! It’s definitely a feature we should have.

@@ -80,6 +80,12 @@ defmodule Orb.Import do
end
end

defmacro register_memory(global, namespace, min \\ nil) do
Copy link
Owner

Choose a reason for hiding this comment

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

Curious why register_memory/3 instead of memory/3?

Copy link
Author

Choose a reason for hiding this comment

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

Just seemed like the thing to do, I was frankly pretty tired when I wrote this so perhaps memory/3 would be more appropriate.

@@ -3,7 +3,7 @@ defmodule Orb.Memory do
Work with memory: load, store, declare pages & initial data.
"""

defstruct name: "", min: 0, exported_name: "memory"
defstruct name: "", min: 0, exported_name: "memory", is_imported?: false
Copy link
Owner

Choose a reason for hiding this comment

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

There‘s a bug from me where we aren’t using the exported_name field in the wat output. So I should fix that.

I think generally it makes sense if you can export xor import, not both. So I’ll have a think about how best to represent that…

Copy link
Author

Choose a reason for hiding this comment

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

Suggested resolution? Or needs some thinking?

Copy link
Owner

Choose a reason for hiding this comment

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

Sorry work has been super busy, I’ll resolve this soon!

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.

3 participants