Stream: git-wasmtime

Topic: wasmtime / PR #2108 Caller get_export() implemented for E...

Wasmtime GitHub notifications bot (Aug 06 2020 at 16:51):

agiachris opened PR #2108 from main to main:

To my knowledge, this has not been discussed in an issue. However, comments in the overwritten code make clear that the get_export() function is only currently implemented for Extern::Memory, with potential future support for Extern::Func - which is the update of this PR. This update simply enables user to lookup and return Extern::Func types from Caller structure.

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [x] This has been discussed in issue #..., or if not, please tell us why
  here.

• [x] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [x] This PR contains test cases, if meaningful.

• [x] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Aug 06 2020 at 17:16):

PiotrSikora submitted PR Review.

Wasmtime GitHub notifications bot (Aug 06 2020 at 17:16):

PiotrSikora created PR Review Comment:

Style: s/return Some(Extern::Func(func));/Some(Extern::Func(func))/ (i.e. no return, no ;).

Wasmtime GitHub notifications bot (Aug 06 2020 at 17:16):

PiotrSikora created PR Review Comment:

Style: s/return Some(Extern::Memory(mem));/Some(Extern::Memory(mem))/ (i.e. no return, no ;).

Wasmtime GitHub notifications bot (Aug 06 2020 at 17:16):

PiotrSikora submitted PR Review.

Wasmtime GitHub notifications bot (Aug 06 2020 at 17:16):

PiotrSikora created PR Review Comment:

Style: s/return None,/None,/ (i.e. no return).

Wasmtime GitHub notifications bot (Aug 06 2020 at 17:45):

agiachris updated PR #2108 from main to main:

To my knowledge, this has not been discussed in an issue. However, comments in the overwritten code make clear that the get_export() function is only currently implemented for Extern::Memory, with potential future support for Extern::Func - which is the update of this PR. This update simply enables user to lookup and return Extern::Func types from Caller structure.

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [x] This has been discussed in issue #..., or if not, please tell us why
  here.

• [x] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [x] This PR contains test cases, if meaningful.

• [x] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Aug 06 2020 at 18:22):

PiotrSikora submitted PR Review.

Wasmtime GitHub notifications bot (Aug 07 2020 at 15:23):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Aug 07 2020 at 15:23):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Aug 07 2020 at 15:23):

alexcrichton created PR Review Comment:

Could this be refactored a bit to avoid duplicating the let handle = ...?

Something like:

let export = instance.lookup(name)?:
let handle = ...;
Some(match export {
    Export::Memory(m) => Export::Memory(Memory::from_wasmtime_memory(m, handle)),
    Export::Function(f) => /* ... */,
    _ => return None,
})
~~~

Wasmtime GitHub notifications bot (Aug 07 2020 at 15:23):

alexcrichton created PR Review Comment:

I think that the documentation on the Caller type itself needs to be updated to indicate it's more than just memory now, and this function can probably point to that documentation to guide about usages of the function.

On the Caller struct itself I think it can be updated to something like:

This structure can be taken as the first parameter of a closure passed to
[Func::wrap], and it can be used to learn information about the caller of
the function, such as the calling module's memory, exports, etc.

The primary purpose of this structure is to provide access to the
caller's information, namely it's exported memory and exported functions. This
allows functions which take pointers as arguments to easily read the memory the
pointers point into, or if a function is expected to call malloc in the wasm
module to reserve space for the output you can do that.

Note that this Caller type a pretty temporary mechanism for accessing the
caller's information until interface types has been fully standardized and
implemented. The interface types proposal will obsolete this type and this will
be removed in the future at some point after interface types is implemented. If
you're relying on this Caller type it's recommended to become familiar with
interface types to ensure that your use case is covered by the proposal.

Wasmtime GitHub notifications bot (Aug 07 2020 at 15:26):

agiachris submitted PR Review.

Wasmtime GitHub notifications bot (Aug 07 2020 at 15:26):

agiachris created PR Review Comment:

Great, will make these updates. Thank you!

Wasmtime GitHub notifications bot (Aug 07 2020 at 15:44):

agiachris updated PR #2108 from main to main:

To my knowledge, this has not been discussed in an issue. However, comments in the overwritten code make clear that the get_export() function is only currently implemented for Extern::Memory, with potential future support for Extern::Func - which is the update of this PR. This update simply enables user to lookup and return Extern::Func types from Caller structure.

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [x] This has been discussed in issue #..., or if not, please tell us why
  here.

• [x] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [x] This PR contains test cases, if meaningful.

• [x] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Aug 07 2020 at 16:24):

alexcrichton merged PR #2108.

Last updated: Nov 22 2024 at 17:03 UTC