wasmtime / PR #4217 Refactor the ComponentValue impls for... · git-wasmtime

In that you're mapping struct Foo(u32, u32) to tuple<u32, u32> in the component model? For that I'd probably just return an error on a struct looking like that since that's already exposed as tuples and there's not really much need to expose a different way to name tuples in Rust.

For enum variants are you supporting enum Foo { A(u32, u32) }? If so personally I'd prefer to reject that for now and stay strict in the derive to stay close to the component model representation if we can.

Depending how the whole ecosystem shakes out we can revisit in the future and add "sugar" if this sort of type structure ends up being common in wit

Wasmtime GitHub notifications bot (Jun 03 2022 at 20:26):

jameysharp created PR review comment:

Huh, I see. Sure, I can do it that way.

I still want to factor this function out of the impl_component_ty_for_tuples macro, even if it isn't public. It's very similar to the typechecking function for unions, and with the addition of names, both of those resemble the typechecking functions for records, enums, and variants. I think it'll be helpful to keep them all next to each other.

I've verified that rustc generates the same code on x86-64 release builds after refactoring the impl for tuples this way. (It inlines the helper function and unrolls the iterator, so the recursive typechecking calls are all direct.) So it seems to me that the main consideration here is whether the refactoring is easier to understand, and for me, getting code out of macros helps a lot in my ability to understand it.

Wasmtime GitHub notifications bot (Jun 03 2022 at 20:26):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Jun 03 2022 at 20:53):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jun 03 2022 at 20:53):

alexcrichton created PR review comment:

Ok that seems reasonable to me yeah. In general we don't have to worry too much about type-checking performance since that's typically front-loaded where possible off the critical path so it's ok to drop #[inline] on functions like this and otherwise just optimize it later if we really need to.

Wasmtime GitHub notifications bot (Jun 05 2022 at 01:09):

jameysharp updated PR #4217 from component-tuple-refactor to main.

Wasmtime GitHub notifications bot (Jun 05 2022 at 01:12):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Jun 05 2022 at 01:12):

jameysharp created PR review comment:

Okay, I fixed typecheck_tuple to not be pub. Thanks for your review! Anything else blocking this from merge?

Wasmtime GitHub notifications bot (Jun 06 2022 at 15:44):

jameysharp merged PR #4217.

Last updated: Dec 23 2024 at 13:07 UTC