Rust cleanup #6789
Rust cleanup #6789
Conversation
|
|
||
| /// 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.
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.
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