Stream: git-wasmtime

Topic: wasmtime / PR #7828 Expose `ModuleExport` to avoid repeat...


view this post on Zulip Wasmtime GitHub notifications bot (Jan 27 2024 at 02:18):

grovesNL edited PR #7828.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 27 2024 at 03:29):

grovesNL edited a comment on PR #7828:

Thanks for clarifying! I updated the PR to match the scheme you described.

Is the purpose of including CompiledModuleId on ModuleExport just out of convenience for user code, or should we compare these somewhere during get_module_export?

view this post on Zulip Wasmtime GitHub notifications bot (Jan 27 2024 at 04:44):

github-actions[bot] commented on PR #7828:

Subscribe to Label Action

cc @peterhuene

<details>
This issue or pull request has been labeled: "wasmtime:api"

Thus the following users have been cc'd because of the following labels:

To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.

Learn more.
</details>

view this post on Zulip Wasmtime GitHub notifications bot (Jan 27 2024 at 21:32):

alexcrichton submitted PR review:

Looks great thanks! Just a few minor comments but otherwise looks good to go.

Is the purpose of including CompiledModuleId on ModuleExport just out of convenience for user code, or should we compare these somewhere during get_module_export?

I commented below as well, but we'll want to check this when looking up exports to prevent this mistake of using the wrong module's exports to index an instance.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 27 2024 at 21:32):

alexcrichton created PR review comment:

Could the above method delegate to this one? Additionally could this one lookup in data.exports to use the cache if it's already filled in? And finally, mind adding a link to Module::get_export_index in the docs of this function?

view this post on Zulip Wasmtime GitHub notifications bot (Jan 27 2024 at 21:32):

alexcrichton submitted PR review:

Looks great thanks! Just a few minor comments but otherwise looks good to go.

Is the purpose of including CompiledModuleId on ModuleExport just out of convenience for user code, or should we compare these somewhere during get_module_export?

I commented below as well, but we'll want to check this when looking up exports to prevent this mistake of using the wrong module's exports to index an instance.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 27 2024 at 21:32):

alexcrichton created PR review comment:

Mind making these private and/or pub(crate)? Ideally users shouldn't have to actually look at these fields.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 27 2024 at 21:32):

alexcrichton created PR review comment:

Also, can this method perform a check that export.module is the same as the module used to create this instance? That prevents a mistake where you get a ModuleExport from one module but look it up in the wrong instance.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 28 2024 at 00:54):

grovesNL updated PR #7828.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 28 2024 at 01:00):

grovesNL commented on PR #7828:

Thank you for the feedback! I updated the PR. I moved the lookup into the shared _get_export function and pass the entity and index to that instead (checking the module ID is only necessary for the new function).

view this post on Zulip Wasmtime GitHub notifications bot (Jan 28 2024 at 01:01):

grovesNL updated PR #7828.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 28 2024 at 01:13):

grovesNL updated PR #7828.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 28 2024 at 02:00):

alexcrichton submitted PR review:

Thanks!

view this post on Zulip Wasmtime GitHub notifications bot (Jan 28 2024 at 02:29):

alexcrichton merged PR #7828.


Last updated: Dec 23 2024 at 13:07 UTC