-
Notifications
You must be signed in to change notification settings - Fork 245
Rust cleanup #6789
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rust cleanup #6789
Conversation
@@ -69,11 +69,26 @@ impl BnString { | |||
} | |||
} | |||
|
|||
/// Take an owned core string and convert it to [`String`]. | |||
/// | |||
/// This expects the passed raw string to be owned, as in, freed by us. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably document the behavior when the input contains non-UTF-8 characters.
let bn_settings_file_name = unsafe { BnString::from_raw(settings_file_name_ptr) }; | ||
PathBuf::from(bn_settings_file_name.to_string()) | ||
let settings_file_path_str = unsafe { BnString::into_string(settings_file_name_ptr) }; | ||
PathBuf::from(settings_file_path_str) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
File system paths are not required to be UTF-8 on all platforms. I don't know whether it matters for these sorts of paths, but it could be handled by going from a BnString
to an OsStr
then to a PathBuf
rather than via String
.
Threw this on the branch, still need to run more tests, I wouldn't spend time looking at it right now. |
CC @mkrasnitski I added you as a co-author to e66b878 seeing as it is effectively a continuation of your PR. Let me know if that is alright, and also if you have any input into this PR. |
Why not just merge #5897? There are no merge conflicts and then you can rebase this PR on top without the string commit. I did a local rebase to test things, and it also let me compare your string commit with mine. They do essentially the same thing except you add a couple lines to the docstring for |
These are never expressed, just adds another unneeded constraint on the impl
We don't do enough with the lifted il != non lifted il to justify the bound. This makes modifying IL much less work as the historical lifted il bound is gone.
Both of these are associated directly to a `BinaryView` and only exist as accessors onto it.
…nstruction `MediumLevelILInstruction` does not yet handle `MLIL_CALL_OUTPUT`, `MLIL_CALL_PARAM`, `MLIL_CALL_PARAM_SSA`, `MLIL_CALL_OUTPUT_SSA`, `MLIL_MEMORY_INTRINSIC_OUTPUT_SSA`, or `MLIL_MEMORY_INTRINSIC_SSA`. Map these to a `NotYetImplemented` kind rather than panicking since a panic takes down the entire app.
And some other misc cleanup
For 1 I think this is pretty easy migration for users, they will just need remove the extra bound, build errors are descriptive enough.
For 2 this just means that users are more responsible for handling invalid UTF-8 which is the correct call in the cases where we cant already just make that assumption, or that a lossy conversion is acceptable.
I need to move on to some other tasks so this PR will likely not grow, I am going to mark this ready for review.
CC: @bdash @rbran
Related: #6784 #6778