wasmtime / PR #7115 c-api: Expose host memory creation · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #7115 c-api: Expose host memory creation

Wasmtime GitHub notifications bot (Sep 29 2023 at 19:10):

rockwotj opened PR #7115 from rockwotj:mem to bytecodealliance:main.

Wasmtime GitHub notifications bot (Sep 29 2023 at 19:10):

rockwotj requested fitzgen for a review on PR #7115.

Wasmtime GitHub notifications bot (Sep 29 2023 at 19:10):

rockwotj requested wasmtime-core-reviewers for a review on PR #7115.

Wasmtime GitHub notifications bot (Sep 29 2023 at 19:10):

rockwotj edited PR #7115.

Wasmtime GitHub notifications bot (Sep 29 2023 at 19:48):

rockwotj updated PR #7115.

Wasmtime GitHub notifications bot (Sep 29 2023 at 19:53):

fitzgen submitted PR review:

Thanks! Just a couple nitpicks below.

Wasmtime GitHub notifications bot (Sep 29 2023 at 19:53):

fitzgen created PR review comment:

The return type in the C header is void* but here we have *mut u8. For consistency, either

this should change to *mut std::ffi::c_void

or the C header should change to uint8_t*

Wasmtime GitHub notifications bot (Sep 29 2023 at 19:53):

fitzgen submitted PR review:

Thanks! Just a couple nitpicks below.

Wasmtime GitHub notifications bot (Sep 29 2023 at 19:53):

fitzgen created PR review comment:

Can you document in the C header's docs for the wasmtime_memory_creator_t that the functions need to be thread safe? Thanks.

Wasmtime GitHub notifications bot (Sep 29 2023 at 20:06):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Sep 29 2023 at 20:06):

alexcrichton created PR review comment:

One suggestion I'd have for this is to have a void* that's passed into new_memory as well, but then that would make it a bit confusing since a different void* is passed to new vs get/grow. In that case I might additionally recommend one of two changes:

Split this structure into "creator callbacks" with new + finalizer and "memory callbacks" with get + grow + finalizer

Move the new_memory callback to the wasmtime_config_host_memory_creator_set function as an argument.

I'm somewhat partial to (1) here, but could go either way (I don't design that many C APIs)

Wasmtime GitHub notifications bot (Sep 29 2023 at 20:24):

rockwotj submitted PR review.

Wasmtime GitHub notifications bot (Sep 29 2023 at 20:24):

rockwotj created PR review comment:

I changed the C header. Nice catch!

Wasmtime GitHub notifications bot (Sep 29 2023 at 21:05):

rockwotj updated PR #7115.

Wasmtime GitHub notifications bot (Sep 29 2023 at 21:05):

rockwotj submitted PR review.

Wasmtime GitHub notifications bot (Sep 29 2023 at 21:05):

rockwotj created PR review comment:

I prefer 1 as well. It is done.

Wasmtime GitHub notifications bot (Sep 29 2023 at 21:06):

rockwotj submitted PR review.

Wasmtime GitHub notifications bot (Sep 29 2023 at 21:06):

rockwotj created PR review comment:

Thanks! Done.

Wasmtime GitHub notifications bot (Sep 29 2023 at 21:06):

rockwotj requested fitzgen for a review on PR #7115.

Wasmtime GitHub notifications bot (Sep 29 2023 at 21:06):

rockwotj requested alexcrichton for a review on PR #7115.

Wasmtime GitHub notifications bot (Sep 29 2023 at 21:13):

rockwotj updated PR #7115.

Wasmtime GitHub notifications bot (Sep 29 2023 at 21:22):

rockwotj updated PR #7115.

Wasmtime GitHub notifications bot (Sep 29 2023 at 21:23):

rockwotj updated PR #7115.

Wasmtime GitHub notifications bot (Sep 29 2023 at 22:22):

alexcrichton submitted PR review:

Thanks for this!

Wasmtime GitHub notifications bot (Oct 01 2023 at 02:30):

rockwotj updated PR #7115.

Wasmtime GitHub notifications bot (Oct 01 2023 at 02:43):

rockwotj updated PR #7115.

Wasmtime GitHub notifications bot (Oct 02 2023 at 17:13):

alexcrichton merged PR #7115.

Last updated: Nov 22 2024 at 17:03 UTC