Mach-O: Parse macOS version#525
Conversation
m4b
left a comment
m4b
left a comment
There was a problem hiding this comment.
The unwraps need to be addressed, before can consider to merge. Additionally, it may be more idiomatic to have this be a version method on a MachO, and returns an Optional<Version>; I don't think we have anywhere else where we have a From method on MachO that returns some type, so it might be a little undiomatic in this crate to do that.
m4b
left a comment
m4b
left a comment
There was a problem hiding this comment.
almost there, just need to address some of the issues I flagged, thank you!
| pub struct Version { | ||
| pub major: u32, | ||
| pub minor: u32, | ||
| pub patch: u32, |
There was a problem hiding this comment.
please document all of this, public things, especially new ones, should be doc'd
| use crate::mach::load_command::CommandVariant; | ||
|
|
||
| if_std! { | ||
| use crate::error; |
There was a problem hiding this comment.
iirc, error is not under std; and I don't think cmp or str or fmt is either, they should be under core?
| // X.Y.Z is encoded in nibbles xxxx.yy.zz | ||
| // 12.6 = 0b0000_0000_0000_1100_0000_0110_0000_0000 | ||
| Self { | ||
| major: (packed & 0b1111_1111_1111_1111_0000_0000_0000_0000u32) >> 16, |
There was a problem hiding this comment.
nit: i think we use hex for other bitmasks in other parts of codebase, might want to be uniform with that
| } | ||
|
|
||
| impl PartialEq for Version { | ||
| fn eq(&self, other: &Self) -> bool { |
There was a problem hiding this comment.
you should be able to just #[derive(PartialEq)
| impl Ord for Version { | ||
| fn cmp(&self, other: &Self) -> Ordering { | ||
| let mao = self.major.cmp(&other.major); | ||
| let mio = self.minor.cmp(&other.minor); | ||
| let pao = self.patch.cmp(&other.patch); | ||
| if mao == Ordering::Equal && mio == Ordering::Equal { | ||
| pao | ||
| } else if mao == Ordering::Equal { | ||
| mio | ||
| } else { | ||
| mao | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl PartialOrd for Version { | ||
| fn partial_cmp(&self, other: &Self) -> Option<Ordering> { | ||
| Some(self.cmp(other)) | ||
| } | ||
| } |
There was a problem hiding this comment.
ditto, you should be able to derive this (and it also doesn't require std)
| .split('.') | ||
| .map(|p| p.parse::<u32>().unwrap_or(0)) | ||
| .take(3) | ||
| .collect::<VecDeque<u32>>(); |
There was a problem hiding this comment.
this allocates a VecDequeu here just to pop the front; i think you can rewrite this to not allocate, and please do that. thank you for updating with fallible methods though.
| } | ||
|
|
||
| impl Mach<'_> { | ||
| pub fn versions(self) -> HashMap<CpuType, Version> { |
There was a problem hiding this comment.
needs docstring
In general, I don't necessarily like a method hanging off of Mach here in this file, as it's not local anymore to Mach. This also allocates a hashmap; I think it would be better to probably return an impl Iterator<(CpuType, Version)>; this way users can allocate if they want, etc.
| use crate::mach::MachO; | ||
| use crate::mach::load_command::CommandVariant; | ||
|
|
||
| if_std! { |
There was a problem hiding this comment.
i think you can remove this entire block once you get rid of the allocations
2 participants
Possible implementation for #514