Stream: git-wasmtime

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

Wasmtime GitHub notifications bot (Jan 27 2024 at 02:18):

grovesNL edited PR #7828.

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?

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:

• peterhuene: wasmtime:api

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

Learn more.
</details>

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.

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?

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.

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.

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.

Wasmtime GitHub notifications bot (Jan 28 2024 at 00:54):

grovesNL updated PR #7828.

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).

Wasmtime GitHub notifications bot (Jan 28 2024 at 01:01):

grovesNL updated PR #7828.

Wasmtime GitHub notifications bot (Jan 28 2024 at 01:13):

grovesNL updated PR #7828.

Wasmtime GitHub notifications bot (Jan 28 2024 at 02:00):

alexcrichton submitted PR review:

Thanks!

Wasmtime GitHub notifications bot (Jan 28 2024 at 02:29):

alexcrichton merged PR #7828.

Last updated: Nov 22 2024 at 17:03 UTC