add assembly parser#9
Merged
qmonnet merged 6 commits intoqmonnet:masterfrom Feb 7, 2017
Merged
Conversation
Uses the combine crate to parse ubpf-style assembly to Instruction structs. A future assembler module will consume these, check that the instruction and operands are valid, and output ebpf::Insn.
Closed
qmonnet
approved these changes
Feb 6, 2017
Owner
qmonnet
left a comment
qmonnet
left a comment
There was a problem hiding this comment.
Hi @rlane, many thanks for this first PR! The parser looks very clean. The minor comments I added below were returned by rust-clippy (Rust linter), would you mind fixing them before we merge? Or I can add a commit once it's merged if you prefer me to do it.
| _ => 1, | ||
| }); | ||
| let hex = | ||
| string("0x").with(many1(hex_digit())).map(|x: String| i64::from_str_radix(&x, 16).unwrap()); |
Owner
There was a problem hiding this comment.
Single line in file that is more than 100 character long, do you think we could split it?
| fn operand<I>(input: I) -> ParseResult<Operand, I> | ||
| where I: Stream<Item = char> | ||
| { | ||
| let register_operand = parser(register).map(|x: i64| Operand::Register(x)); |
Owner
There was a problem hiding this comment.
$ cargo clippy -- -A doc_markdown -A match_same_arms
[...]
warning: redundant closure found, #[warn(redundant_closure)] on by default
--> src/asm_parser.rs:66:49
|
66 | let register_operand = parser(register).map(|x: i64| Operand::Register(x));
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
help: remove closure as shown:
| let register_operand = parser(register).map(Operand::Register);
= help: for further information visit https://github.com/Manishearth/rust-clippy/wiki#redundant_closure
| where I: Stream<Item = char> | ||
| { | ||
| let register_operand = parser(register).map(|x: i64| Operand::Register(x)); | ||
| let immediate = parser(integer).map(|x: i64| Operand::Integer(x)); |
Owner
There was a problem hiding this comment.
$ cargo clippy -- -A doc_markdown -A match_same_arms
[...]
warning: redundant closure found, #[warn(redundant_closure)] on by default
--> src/asm_parser.rs:67:41
|
67 | let immediate = parser(integer).map(|x: i64| Operand::Integer(x));
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
help: remove closure as shown:
| let immediate = parser(integer).map(Operand::Integer);
= help: for further information visit https://github.com/Manishearth/rust-clippy/wiki#redundant_closure
| fn format_info(info: &Info<char, &str>) -> String { | ||
| match *info { | ||
| Info::Token(x) => format!("{:?}", x), | ||
| Info::Range(ref x) => format!("{:?}", x), |
Owner
There was a problem hiding this comment.
$ cargo clippy -- -A doc_markdown -A match_same_arms
[...]
warning: this pattern creates a reference to a reference, #[warn(needless_borrow)] on by default
--> src/asm_parser.rs:92:21
|
92 | Info::Range(ref x) => format!("{:?}", x),
| ^^^^^
|
= help: for further information visit https://github.com/Manishearth/rust-clippy/wiki#needless_borrow
| Info::Token(x) => format!("{:?}", x), | ||
| Info::Range(ref x) => format!("{:?}", x), | ||
| Info::Owned(ref x) => x.clone(), | ||
| Info::Borrowed(ref x) => x.to_string(), |
Owner
There was a problem hiding this comment.
$ cargo clippy -- -A doc_markdown -A match_same_arms
[...]
warning: this pattern creates a reference to a reference, #[warn(needless_borrow)] on by default
--> src/asm_parser.rs:94:24
|
94 | Info::Borrowed(ref x) => x.to_string(),
| ^^^^^
|
= help: for further information visit https://github.com/Manishearth/rust-clippy/wiki#needless_borrow
(Note we do need to keep the ref on line 93, though.)
qmonnet
reviewed
Feb 6, 2017
| pub mod disassembler; | ||
| pub mod ebpf; | ||
| pub mod helpers; | ||
| pub mod asm_parser; |
Owner
There was a problem hiding this comment.
Ah also, can we keep includes in alphabetical order (in each one of the crate/public module/private module section)? Also, for dependencies in the Cargo.toml file?
Contributor
Author
|
Thanks for the pointer to rust-clippy. Pushed a few commits. |
Owner
|
Thanks a lot! Merging. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Uses the combine crate to parse ubpf-style assembly to Instruction
structs. A future assembler module will consume these, check that
the instruction and operands are valid, and output ebpf::Insn.