Stream: git-wasmtime

Topic: wasmtime / PR #3086 c-api: add wasmtime_trap_code

Wasmtime GitHub notifications bot (Jul 14 2021 at 12:28):

srenatus opened PR #3086 from sr/c-api/add-trap-code-accessor to main:

Eventually this should be added to the wasmtime-go binding, addressing
https://github.com/bytecodealliance/wasmtime-go/issues/63.

:question: How would this be tested, short of actually using it from a binding? Happy to add test cases, but I'm afraid I'll need some direction.

Wasmtime GitHub notifications bot (Jul 14 2021 at 15:36):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jul 14 2021 at 15:36):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jul 14 2021 at 15:36):

alexcrichton created PR review comment:

I think here it's ok to skip the enum { .. } wrapper, and I think the value of each #define constant will need to be listed.

Wasmtime GitHub notifications bot (Jul 14 2021 at 15:36):

alexcrichton created PR review comment:

Could you leave a comment near enum TrapCode and its #[non_exhaustive] that any additions should get added here too?

Wasmtime GitHub notifications bot (Jul 14 2021 at 15:36):

alexcrichton created PR review comment:

Could this describe a situation where false is returned? (e.g. the trap was originally created with wasmtime_trap_new)

Wasmtime GitHub notifications bot (Jul 14 2021 at 19:31):

srenatus updated PR #3086 from sr/c-api/add-trap-code-accessor to main.

Wasmtime GitHub notifications bot (Jul 14 2021 at 19:31):

srenatus submitted PR review.

Wasmtime GitHub notifications bot (Jul 14 2021 at 19:31):

srenatus created PR review comment:

Oops, I had a bunch of #defines first, then decided to model this after the config enums like

https://github.com/bytecodealliance/wasmtime/blob/73fd702bb7cabdff156eaf4ef8db5b92b098583e/crates/c-api/include/wasmtime/config.h#L56-L64

...and didn't follow through properly :grimacing: I've updated the code using a proper enum instead of defines. What do you think?

Wasmtime GitHub notifications bot (Jul 14 2021 at 19:31):

srenatus submitted PR review.

Wasmtime GitHub notifications bot (Jul 14 2021 at 19:31):

srenatus created PR review comment:

Yup will do

Wasmtime GitHub notifications bot (Jul 14 2021 at 20:51):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jul 14 2021 at 20:51):

alexcrichton created PR review comment:

Eh I could go either way, I don't actually know enough about C to know whether these are really that different, and they feel like the same to me. I'm happy to defer to whichever you feel is more appropriate here!

Wasmtime GitHub notifications bot (Jul 15 2021 at 11:11):

srenatus updated PR #3086 from sr/c-api/add-trap-code-accessor to main.

Wasmtime GitHub notifications bot (Jul 15 2021 at 11:13):

srenatus submitted PR review.

Wasmtime GitHub notifications bot (Jul 15 2021 at 11:13):

srenatus created PR review comment:

I'm not a C expert either -- it felt to be a tad more consistent going with the enum definition. Updated :check_mark:️

Wasmtime GitHub notifications bot (Jul 15 2021 at 11:16):

srenatus updated PR #3086 from sr/c-api/add-trap-code-accessor to main.

Wasmtime GitHub notifications bot (Jul 15 2021 at 11:16):

srenatus has marked PR #3086 as ready for review.

Wasmtime GitHub notifications bot (Jul 15 2021 at 14:30):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jul 15 2021 at 15:31):

alexcrichton merged PR #3086.

Last updated: Nov 22 2024 at 17:03 UTC