Stream: git-wasmtime

Topic: wasmtime / issue #4908 ISLE: add support for multi-extrac...

view this post on Zulip Wasmtime GitHub notifications bot (Sep 14 2022 at 01:42):

github-actions[bot] commented on issue #4908:

Subscribe to Label Action

cc @cfallin, @fitzgen

<details>
This issue or pull request has been labeled: "cranelift", "isle"

Thus the following users have been cc'd because of the following labels:

To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.

Learn more.
</details>

view this post on Zulip Wasmtime GitHub notifications bot (Sep 19 2022 at 21:27):

cfallin commented on issue #4908:

Updated, thanks!

view this post on Zulip Wasmtime GitHub notifications bot (Sep 20 2022 at 20:17):

jameysharp commented on issue #4908:

I still need to read this more carefully, but I think it looks good.

One question so far: what's the intended difference between returning Some with an empty iterator, versus returning None? Put another way, what would we lose if we remove the Option wrapper from those return types? I understand the use of Option for fallible non-multi extractors, but I'm not sure how that extends here. I'd prefer to just use an empty iterator to indicate that the extractor failed, if that's not too much trouble, because otherwise the types appear to permit two ways of encoding the same situation.

I don't see any documentation for how someone writing an external multi-extractor should implement the auto-generated traits. I don't think that's strictly necessary before merging this, but if that documentation existed then I would look for the answer to the above question there.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 20 2022 at 20:20):

cfallin commented on issue #4908:

One question so far: what's the intended difference between returning Some with an empty iterator, versus returning None?

That's a good question. Functionally there is no difference, and the main reason is just to manage complexity: otherwise we have a special case wherever we have a match failure in that we have to invent an empty iterator rather than return None. If it were one code-generator site then maybe that's fine, but we also use the ? operator everywhere and I didn't want to work out how to get auto-conversions going with that. It's simpler to stick to the invariant that all constructors live in the Option monad and let the Try magic do its thing.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 21 2022 at 23:28):

jameysharp commented on issue #4908:

I asked Chris to show me a diff of the generated code for existing uses of ISLE. We verified that the only difference was additional types and traits related to ContextIter. As a result I'm convinced that this is safe to merge as-is. I haven't fully understood the multi-extractors and multi-constructors, but since we don't use any of those yet, we can fix any issues that come up when we start trying to use them, instead of worrying about them now.

Last updated: Nov 22 2024 at 16:03 UTC