Rewrite fuel_asm::impl_instructions! to a proc macro #804
Conversation
fuel-derive/src/asm.rs
Outdated
| let name = &arg.name; | ||
| if arg.is_imm() { | ||
| quote! { | ||
| packed_integer |= (#name.to_smallest_int() as u32); |
There was a problem hiding this comment.
Places like this would be better with a single-use helper function.
There was a problem hiding this comment.
Moved most of these to smaller functions.
fuel-derive/src/asm.rs
Outdated
| .0 | ||
| .iter() | ||
| .map( | ||
| |Instruction { |
There was a problem hiding this comment.
It would be nice for each closure that we use inside of the map to have a named function like make_constructor_for_opcode. Name is up to you=)
There was a problem hiding this comment.
I've simplified the code quite a bit and each map has only a small block of code inside. Let me know if you still feel we need to have the closures as separate functions as well.
The PR that you linked is pure refactoring, where we moved one part of the code to another part of the code. While in this PR, we re-implement the macro rule via the procedure macro. It is newly written logic. We need to carefully review the implementation and compare the old expanded code with the new one to be sure that everything is correct. Also, maybe we want to have an audit of this re-writing. I still need to think about the main benefits of using procedural macro vs macro rule. I like the macro rule more because you can debug in your IDE there, but you can't do the same for the generated code from the procedure macro. But at the same time, the macro rule requires a lot of computation, and sometimes, my IDE freezes evaluating our macro rule for opcodes. |
|
Okey, fair. |
We could instead publish the code produced by expanding the macro, and not ship a proc macro at all. The produced code is reasonably small and shouldn't have any ot these issues. I find the declarative macro to be unreadable and error-prone. As long as we don't change anything it's fine though. |
|
That all said, I'm closing this and #805 for now. We can reopen if we need to modify the macros in the future. |
What it says on the tin. No exteranal functional changes, just refactoring.
Crate-internal
test_constructfunction has been removed, as the associated test didn't seem to catch any errors. It has been rewritten to use a fuzzing-style property-based testing.The purpose of this PR is to allow more complex instruction definitions in the future. This should allow moving some validity checks to instruction parsing level, and to improve type safety.
Checklist
Before requesting review