Regs access #149
Regs access #149
Conversation
|
Also @tmfink, whats the current status on the project? Seems like it's missing some updates? I could possibly step in and try and get things up to date. |
See the review comments (to come).
64 is a fine value, but should probably be defined as a constant (magic numbers are not self-describing). Looks like this would fix #63. |
| to_vec(regs_write, regs_write_count as usize), | ||
| ) | ||
| })) | ||
| } else { |
There was a problem hiding this comment.
use an early return in the case of an error to reduce the indentation of the "happy" case
There was a problem hiding this comment.
are you referring to the entire outer if cfg!(...) {? I agree it's cleaner, however the entire file is written this way currently :)
capstone-rs/src/capstone.rs
Outdated
| @@ -365,6 +365,44 @@ impl Capstone { | |||
| } | |||
| } | |||
|
|
|||
| /// Get the registers are which are read to and written to, in that order. | |||
| pub fn regs_access(&self, insn: &Insn) -> Option<CsResult<(Vec<RegId>, Vec<RegId>)>> { | |||
There was a problem hiding this comment.
I don't like forcing allocations here by returning Vec. You can keep an allocation version for convenience, but please also create a regs_access_buf() where the user can provide their own buffers (which may just be stack allocated).
For simplicity, the regs_access() can call regs_access_buf()
There was a problem hiding this comment.
Should I just append the registers to the provided buffers, or should i clear them first?
There was a problem hiding this comment.
If the user may provide stack allocated buffers, doesn't that mean they generally have to provide the length aswell? If they can pass a regs_read: &mut [u16; MAX_NUM_REGISTERS], how will we specify how many were actually read?
capstone-rs/src/capstone.rs
Outdated
| @@ -365,6 +365,44 @@ impl Capstone { | |||
| } | |||
| } | |||
|
|
|||
| /// Get the registers are which are read to and written to, in that order. | |||
| pub fn regs_access(&self, insn: &Insn) -> Option<CsResult<(Vec<RegId>, Vec<RegId>)>> { | |||
There was a problem hiding this comment.
Instead of using a tuple, please make a new struct that makes it clear what is the read and write fields:
pub struct RegAccess {
read: Vec<RegId>,
write: Vec<RegId>,
}The return type would be CsResult<RegAccess>.
There was a problem hiding this comment.
I used that struct but made the fields public, is it good now?
I'll file a separate issue to address this question. To update the internal capstone version, please:
|
Codecov Report
@@ Coverage Diff @@
## master #149 +/- ##
==========================================
- Coverage 93.99% 93.98% -0.01%
==========================================
Files 22 22
Lines 2731 2728 -3
Branches 2685 2682 -3
==========================================
- Hits 2567 2564 -3
Misses 164 164
... and 3 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
I just filed #151. |
I think the if cfg! is kinda weird and should probably rather have it just gate the entire function instead? Anyway, I'm not sure how to handle
assert!(len <= ints.len());better, but it should never happen anyway so it probably doesn't matter?Also I just picked 64 because python uses that, though I have a hard time imagining reading/writing to more than 64 registers: https://github.com/capstone-engine/capstone/blob/5fb8a423d4455cade99b12912142fd3a0c10d957/bindings/python/capstone/__init__.py#L929