Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

workspace: Remove wasmparser::Operator type usage #711

Closed
3 of 8 tasks
Robbepop opened this issue Aug 11, 2022 · 1 comment
Closed
3 of 8 tasks

workspace: Remove wasmparser::Operator type usage #711

Robbepop opened this issue Aug 11, 2022 · 1 comment

Comments

@Robbepop
Copy link
Collaborator

Robbepop commented Aug 11, 2022

With #697 merged it is possible to remove the Operator type from the wasmparser crate.

The new VisitOperator trait allows to parse and validate Wasm without the need for matching on an Operator. Instead respective visit methods are invoked which avoid unnecessary unpredictable branches and is therefore the preferred and recommended API to use for high-performance Wasm parsing and validation.

This issue tracks progress and discussion for removing the Operator type.

cc @alexcrichton

ToDo

alexcrichton added a commit to alexcrichton/wasm-tools that referenced this issue Aug 11, 2022
alexcrichton added a commit to alexcrichton/wasm-tools that referenced this issue Aug 15, 2022
alexcrichton added a commit that referenced this issue Aug 15, 2022
* Remove usage of `Operator` in `wasm-tools dump`

cc #711

* Remove `op:` prefix
alexcrichton added a commit to alexcrichton/wasm-tools that referenced this issue Aug 16, 2022
This also reorders methods in the trait definition to match the order
they're defined in the spec to try to organize things together a bit
more.

cc bytecodealliance#711
alexcrichton added a commit that referenced this issue Aug 17, 2022
This also reorders methods in the trait definition to match the order
they're defined in the spec to try to organize things together a bit
more.

cc #711
alexcrichton added a commit to alexcrichton/wasm-tools that referenced this issue Aug 18, 2022
This commit updates the `check_const_expr` function to remove the usage
of the `Operator` visitor in the spirit of eventually removing the
`Operator` type if possible. This was achieved through some macro-magic
and isn't necessarily more readable than the prior version, so I could
see this going either way.

In the long run with bytecodealliance#733 it's not really difficult per-se to maintain
an `Operator` interface in `wasmparser` since it's trivially defined via
macros. This may be a case where we want to use that more than the
`visit_*` pieces perhaps.

cc bytecodealliance#711
fitzgen pushed a commit that referenced this issue Aug 18, 2022
This commit updates the `check_const_expr` function to remove the usage
of the `Operator` visitor in the spirit of eventually removing the
`Operator` type if possible. This was achieved through some macro-magic
and isn't necessarily more readable than the prior version, so I could
see this going either way.

In the long run with #733 it's not really difficult per-se to maintain
an `Operator` interface in `wasmparser` since it's trivially defined via
macros. This may be a case where we want to use that more than the
`visit_*` pieces perhaps.

cc #711
@Robbepop Robbepop changed the title wasmparser: Remove Operator type workspace: Remove Operator type Aug 19, 2022
@Robbepop Robbepop changed the title workspace: Remove Operator type workspace: Remove Operator type usage Aug 19, 2022
@Robbepop Robbepop changed the title workspace: Remove Operator type usage workspace: Remove wasmparser::Operator type usage Aug 19, 2022
@alexcrichton
Copy link
Member

I'm going to actually go ahead and close this despite not being fully finished yet. With the for_each_operator! macro it's trivial to maintain both an enum-based representation in addition to the trait-based visitor. Some use cases I think are still better expressed with an Operator enum than the trait (so long as performance isn't a concern), so it seems reasonable to me to have both in this library.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants