Implement code parser#22
Conversation
Palmr
left a comment
Palmr
left a comment
There was a problem hiding this comment.
In general I like it. My comments on the code are all stylistic really and don't have any bearing on the function.
If you address those comments I'd be happy to merge.
It would also be nice to add more tests (tedious as it is, one per opcode) to check each opcode decodes to the correct instruction and parses arguments, if applicable, correctly. Currently it looks like you only added a few of the more interesting opcodes to your tests.
This could be done later though if you create an issue "Add instruction parsing tests" to track it.
| 0x21 => value!(Instruction::Lload3) | | ||
| 0x69 => value!(Instruction::Lmul) | | ||
| 0x75 => value!(Instruction::Lneg) | | ||
| 0xab => do_parse!(apply!(align, address + 1) >> default: be_i32 >> npairs: be_u32 >> pairs: count!(do_parse!(lookup: be_i32 >> offset: be_i32 >> (lookup, offset)), npairs as usize) >> (Instruction::Lookupswitch{default:default, pairs: pairs})) | |
There was a problem hiding this comment.
This line is too long, the parser is doing too much. Perhaps turn it into its own function rather than keeping it inline.
| 0x56 => value!(Instruction::Sastore) | | ||
| 0x11 => map!(be_i16, |value| Instruction::Sipush(value)) | | ||
| 0x5f => value!(Instruction::Swap) | | ||
| 0xaa => do_parse!(apply!(align, address + 1) >> default: be_i32 >> low: be_i32 >> high: be_i32 >> offsets: count!(be_i32, (high-low+1) as usize) >> (Instruction::Tableswitch{default:default, low:low, high:high, offsets:offsets})) | |
There was a problem hiding this comment.
Same thing as for line 195, this line is doing too much and should be broken down.
| 0x32 => value!(Instruction::Aaload) | | ||
| 0x53 => value!(Instruction::Aastore) | | ||
| 0x01 => value!(Instruction::Aconstnull) | | ||
| 0x19 => map!(be_u8, |index| Instruction::Aload(index)) | |
There was a problem hiding this comment.
There are many lines here following the same pattern, mapping where the closure calls a function that takes the same parameter.
These can be simplified from:
map!(be_u8, |index| Instruction::Aload(index))to
map!(be_u8, Instruction::Aload)See: https://rust-lang.github.io/rust-clippy/master/#redundant_closure
george-hopkins
force-pushed
the
code-parser
branch
from
6070fee to
9a18d6a
Compare
|
Thank you for the review. I tried to address the issues you mentioned. As for the testcases, they are quite small indeed. So far, I haven't found a good corpus of all opcodes which we could use. If it's ok with you, I will create a new ticket and leave it open for the time being. |
|
A new ticket to track adding test cases sounds okay to me. Looks like you've addressed all the review comments so will merge this change. Thanks for your contribution @george-hopkins |
|
Thank you for merging! You can find the tracking issue here: #24 |
2 participants
This patch contains the implementation of a code parser. It should be able to handle the current Java instruction set. Except for
lookupswitchandtableswitch(which have some alignment requirements), the parser basically just needs to look at the first byte to decode the instruction.Feel free to close this pull-request if you think code parsing should better be handled separate package.