Simplify arity implementation #2236
Conversation
Originally the annotations in the `for_each_operator!` macro were intended to be used to implement the validator in addition to arity, but during the PR this part was removed and the annotations were only used for arity. As such the macro/annotations were a bit more complicated than they might otherwise need to be. Coupled with bytecodealliance#2235 where some changes may be needed to arity calculations the macro was a bit difficult to understand. As a result this commit simplifies the arity annotations to either: * `arity N -> N` * `arity custom` All "custom" instructions are now written as Rust functions in the `arity.rs` module. They delegate to each other internally as appropriate in a similar manner to how the macro calculated before. Overall this passes tests but I can clarify that I only have a tenuous understanding at best of the previous macro. I did my best to faithfully translate it from what it used to be but it's possible I got something wrong along the way.
|
On changing the macro, no real objection from me if you prefer this style. I did think there was some elegance in being able to define the arity of each instruction in terms of a small number of composable primitives, e.g.: Block { blockty: $crate::BlockType } => visit_block (arity block -> ~block)`
Loop { blockty: $crate::BlockType } => visit_loop (arity block -> ~block)
If { blockty: $crate::BlockType } => visit_if (arity 1 block -> ~block)vs. Block { blockty: $crate::BlockType } => visit_block (arity custom)
Loop { blockty: $crate::BlockType } => visit_loop (arity custom)
If { blockty: $crate::BlockType } => visit_if (arity custom)and having a separate custom function for each one of these. But I think this is a matter of taste, and I'm new to Rust, so fine with me if you want to change it to a less macro-heavy approach. What I would prudentially maybe avoid is changing the FuncValidator code (that tests the correctness of the arity computation) at the same time as changing the arity computation itself. Although I realize it's a pretty small change here so the risk is low. |
|
@keithw for me and historically my experience in Rust has been that heavy usage of macros can lead to difficult-to-understand code so I try to avoid it where possible, and then if a big macro is necessary I try to keep it as simple as possible. In that sense I do agree with your sentiments but the way I would frame it is that the building blocks are the Also agreed on not changing |
Originally the annotations in the
for_each_operator!macro were intended to be used to implement the validator in addition to arity, but during the PR this part was removed and the annotations were only used for arity. As such the macro/annotations were a bit more complicated than they might otherwise need to be. Coupled with #2235 where some changes may be needed to arity calculations the macro was a bit difficult to understand. As a result this commit simplifies the arity annotations to either:arity N -> Narity customAll "custom" instructions are now written as Rust functions in the
arity.rsmodule. They delegate to each other internally as appropriate in a similar manner to how the macro calculated before.Overall this passes tests but I can clarify that I only have a tenuous understanding at best of the previous macro. I did my best to faithfully translate it from what it used to be but it's possible I got something wrong along the way.