You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Currently the Operator enum has a lifetime attached which makes it a bit harder to work with in some contexts.
The only reason for this is its BrTable { table: BrTable<'a> } variant with which it is possible as user of this crate to lazily (but necessarily) parse the targets of the branching table.
Proposed Design
The solution I wanted to propose here is to split the single Operator::BrTable variant into 3 variants:
Operator::BrTableStart { count_targets: u32 }
Operator::BrTableTarget { depth: u32 }
Operator::BrTableDefault { depth: u32 }
These variants will appear in exactly this order for every BrTable in the Wasm input if parsed by the wasmparser crate.
The count_targets: u32 field tells the user how many Operator::BrTableTarget variants are to be expected directly afterwards, followed by a single Operator::BrTableDefault variant.
Alternative Design
An alternative design would be to treat the Operator::BrTableDefault as just another Operator::BrTableTarget and therefore the count_targets: u32 field in BrTableStart simply reflects the number of all targets as well as the additional default target.
What does this solve?
This proposal would eliminate the lifetime in the very big Operator enum. However, it would also be a breaking change and would probably result in potentially significant changes in how users handle BrTable inputs.
Additional benefits are that the Operator enum could then more or less easily derive a lot more useful traits such as:
Copy
PartialEq and Eq
PartialOrd and Ord
Hash
Also this proposal solves the current double parsing of BrTable as convenience type.
The current implementation first parses the br_table just for validating the structure but throws all the information away so that the user can use this convenience type to re-parse and store the information. With this proposal the double parsing would be gone.
Streamlined Design
In my opinion this design would streamline the design of the Operator enum since it already splits some entities into multiple operators such as If, Else and End or Block and End or Loop and End etc. The split of BrTable would be just another.
Currently the
Operator
enum has a lifetime attached which makes it a bit harder to work with in some contexts.The only reason for this is its
BrTable { table: BrTable<'a> }
variant with which it is possible as user of this crate to lazily (but necessarily) parse the targets of the branching table.Proposed Design
The solution I wanted to propose here is to split the single
Operator::BrTable
variant into 3 variants:Operator::BrTableStart { count_targets: u32 }
Operator::BrTableTarget { depth: u32 }
Operator::BrTableDefault { depth: u32 }
These variants will appear in exactly this order for every
BrTable
in the Wasm input if parsed by thewasmparser
crate.The
count_targets: u32
field tells the user how manyOperator::BrTableTarget
variants are to be expected directly afterwards, followed by a singleOperator::BrTableDefault
variant.Alternative Design
An alternative design would be to treat the
Operator::BrTableDefault
as just anotherOperator::BrTableTarget
and therefore thecount_targets: u32
field inBrTableStart
simply reflects the number of all targets as well as the additional default target.What does this solve?
This proposal would eliminate the lifetime in the very big
Operator
enum. However, it would also be a breaking change and would probably result in potentially significant changes in how users handleBrTable
inputs.Additional benefits are that the
Operator
enum could then more or less easily derive a lot more useful traits such as:Copy
PartialEq
andEq
PartialOrd
andOrd
Hash
Also this proposal solves the current double parsing of
BrTable
as convenience type.The current implementation first parses the
br_table
just for validating the structure but throws all the information away so that the user can use this convenience type to re-parse and store the information. With this proposal the double parsing would be gone.Streamlined Design
In my opinion this design would streamline the design of the
Operator
enum since it already splits some entities into multiple operators such asIf
,Else
andEnd
orBlock
andEnd
orLoop
andEnd
etc. The split ofBrTable
would be just another.Existing Works
It seems that the widely used
wasmi
interpreter is already using a similar technique to handle branching tables as can be seen here: (Please note that I am not familiar with its codebase.)https://github.com/paritytech/wasmi/blob/master/src/isa.rs#L359
I am curious if this idea has already been discussed and what others think about it.
The text was updated successfully, but these errors were encountered: