Milek7 opened PR #7854 from Milek7:guestprofiler-c-api
to bytecodealliance:main
.
Milek7 requested alexcrichton for a review on PR #7854.
Milek7 requested wasmtime-core-reviewers for a review on PR #7854.
Sorry for the noise with #7853, I messed up with Git.
github-actions[bot] commented on PR #7854:
Subscribe to Label Action
cc @peterhuene
<details>
This issue or pull request has been labeled: "wasmtime:c-api"Thus the following users have been cc'd because of the following labels:
- peterhuene: wasmtime:c-api
To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.
Learn more.
</details>
alexcrichton submitted PR review:
Thanks for the PR here, and looks good to me! I've got some small suggestions here and there but let me know if I can clarify or help out.
alexcrichton submitted PR review:
Thanks for the PR here, and looks good to me! I've got some small suggestions here and there but let me know if I can clarify or help out.
alexcrichton created PR review comment:
A small suggestion might be to put
modules_size
after themodules_name
andmodules_module
pointers since I think other parts of the C API typically do pointer-then-length. (but please correct me if I'm misremembering)
alexcrichton created PR review comment:
Mind adding some documentation for this and the functions below? One thing I think is especially important to document is the ownership of pointers and who's responsible for deallocating what, for example.
alexcrichton created PR review comment:
This I think would best be modelled as
profiler: Box<wasm_guestprofiler_t>
rather than usingMaybeUninit
. That way this could be a safe function as well.
Milek7 updated PR #7854.
Milek7 submitted PR review.
Milek7 created PR review comment:
Seems mixed, eg. wasm_byte_vec_new takes size first. I can change this but I feel that because this length applies to two following arguments it's clearer this way.
Milek7 updated PR #7854.
Milek7 submitted PR review.
Milek7 created PR review comment:
On second though, it might be better to define
struct {wasm_name_t*, wasmtime_module_t*}
and pass pointer to that?
Milek7 edited PR review comment.
Milek7 updated PR #7854.
Milek7 submitted PR review.
Milek7 created PR review comment:
I changed to passing tuple as struct instead of column-major pointers.
Milek7 submitted PR review.
Milek7 created PR review comment:
This is named
mod
instead ofmodule
because doxygen bugs out otherwise.
Milek7 updated PR #7854.
Milek7 submitted PR review.
Milek7 created PR review comment:
Yep, I have no idea what I'm doing in Rust. It must be Box otherwise it would leave allocation which isn't really possible to clean up legally from C side.
Milek7 updated PR #7854.
I think I addressed everything for now.
Milek7 requested alexcrichton for a review on PR #7854.
alexcrichton submitted PR review:
Thanks for this!
Milek7 updated PR #7854.
Milek7 requested alexcrichton for a review on PR #7854.
alexcrichton merged PR #7854.
Last updated: Dec 23 2024 at 13:07 UTC