capstone-rust / capstone-rs

high-level Capstone system bindings for Rust
217 stars 75 forks source link

impl From<u32> for instructions enums #79

Closed losynix closed 4 years ago

losynix commented 4 years ago

Addresses issue https://github.com/capstone-rust/capstone-rs/issues/56 with your solution 4: Add a function to X86Insn (and other archs) to create an X86Insn from an InsnId.

Now mothran's example can be written like this:

fn is_valid_ret(insn: &Insn) -> bool {
    match X86Insn::from_insn_id(insn.id().0) {
        X86Insn::X86_INS_RET => true,
        _ => false
    }
}

Parsing and appending code to the generated bindings is a bit dirty, maybe Rust will have a good and safe way to cast integers to enums one day.

tmfink commented 4 years ago

Thanks for the PR! I have avoided doing this kind of thing, but maybe is what we want. I have made some review comments.

codecov[bot] commented 4 years ago

Codecov Report

Merging #79 into master will increase coverage by 0.02%. The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #79      +/-   ##
==========================================
+ Coverage   93.85%   93.87%   +0.02%     
==========================================
  Files          20       20              
  Lines        2700     2742      +42     
==========================================
+ Hits         2534     2574      +40     
- Misses        166      168       +2
Impacted Files Coverage Δ
capstone-rs/src/error.rs 63.41% <0%> (-1.59%) :arrow_down:
capstone-rs/src/test.rs 96% <0%> (-0.01%) :arrow_down:
capstone-rs/src/arch/x86.rs 100% <0%> (ø) :arrow_up:
capstone-rs/src/arch/m680x.rs 95.48% <0%> (+0.03%) :arrow_up:
capstone-rs/src/arch/arm.rs 90.47% <0%> (+0.06%) :arrow_up:
capstone-rs/src/arch/tms320c64x.rs 97.67% <0%> (+0.07%) :arrow_up:
capstone-rs/src/arch/mod.rs 90.9% <0%> (+0.08%) :arrow_up:
capstone-rs/src/constants.rs 79.76% <0%> (+0.24%) :arrow_up:
capstone-rs/src/arch/m68k.rs 92.05% <0%> (+0.26%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 2ba6f9a...49b9979. Read the comment docs.

losynix commented 4 years ago

Thank you, I updated the code following your comments except for the TryFrom trait implementation which requires returning a Result (thus an Error instead of a <arch>_INS_INVALID variant), not sure what would be more comfortable to use ?

tmfink commented 4 years ago

Thank you, I updated the code following your comments except for the TryFrom trait implementation which requires returning a Result (thus an Error instead of a <arch>_INS_INVALID variant), not sure what would be more comfortable to use ?

You can just use the From trait to convert directly, including the <arch>_INS_INVALID.

In the future, I think it will make sense to define a separate enum for each arch instructions without <arch>_insns_invalid. When conversions fail, they would be able to return Option<ArchInsn>.

losynix commented 4 years ago

You can just use the From trait to convert directly, including the <arch>_INS_INVALID.

Sure, done.

In the future, I think it will make sense to define a separate enum for each arch instructions without <arch>_insns_invalid. When conversions fail, they would be able to return Option<ArchInsn>.

It would make sense indeed.

losynix commented 4 years ago

everything good for you ?

tmfink commented 4 years ago

Thanks for the PR! Sorry for the wait.

losynix commented 4 years ago

no problem, thank you !