Stream: git-wasmtime

Topic: wasmtime / issue #1056 VerifierStepResult is confusing

Wasmtime GitHub notifications bot (May 04 2022 at 20:37):

cfallin labeled issue #1056:

The verifier functions have an API that could probably be simplified. Functions which return VerifierStepResult must by convention also take an out-param VerifierErrors that will contain non-fatal errors.

Moreover, the T in VerifierStepResult<T> seems to always be set to (), so it's unused.

It seems the out param is redundant with the error that's present in the Result hidden VerifierStepResult. I think slightly modifying the interface of VerifierStepResult would avoid this out param:

• let the Ok type be VerifierErrors, in case we only have non-fatal errors (they could be called "warnings").
• let the Err type stay the same (VerifierErrors too), and contain non-fatal and fatal errors, if there was at least one fatal error.

Then we wouldn't need the errors outparam anymore, which looks cleaner and "more Rusty". It might mean that a few users of the fatal! etc macros would need to have their own errors variable, but that seems OK.

Thoughts?

Wasmtime GitHub notifications bot (May 04 2022 at 20:37):

cfallin labeled issue #1056:

The verifier functions have an API that could probably be simplified. Functions which return VerifierStepResult must by convention also take an out-param VerifierErrors that will contain non-fatal errors.

Moreover, the T in VerifierStepResult<T> seems to always be set to (), so it's unused.

It seems the out param is redundant with the error that's present in the Result hidden VerifierStepResult. I think slightly modifying the interface of VerifierStepResult would avoid this out param:

• let the Ok type be VerifierErrors, in case we only have non-fatal errors (they could be called "warnings").
• let the Err type stay the same (VerifierErrors too), and contain non-fatal and fatal errors, if there was at least one fatal error.

Then we wouldn't need the errors outparam anymore, which looks cleaner and "more Rusty". It might mean that a few users of the fatal! etc macros would need to have their own errors variable, but that seems OK.

Thoughts?

Wasmtime GitHub notifications bot (Oct 05 2024 at 20:38):

aidenfoxivey commented on issue #1056:

@cfallin This issue seems finished - could it be a candidate for being closed?

Wasmtime GitHub notifications bot (Oct 05 2024 at 20:43):

cfallin commented on issue #1056:

I don't think the core issue is actually resolved at all; and in the linked PR it mentions that it resolved a subpart of the problem only. This could still be a good starter-issue for someone wanting to do some cleanups.

Wasmtime GitHub notifications bot (Oct 05 2024 at 20:52):

aidenfoxivey commented on issue #1056:

I don't think the core issue is actually resolved at all; and in the linked PR it mentions that it resolved a subpart of the problem only. This could still be a good starter-issue for someone wanting to do some cleanups.

Ah neat, I see. I'll give it a read. :)

Last updated: Oct 23 2024 at 20:03 UTC