manticore
manticore copied to clipboard
[protocol] Parse manifest flags into structs
fixes #60
Hi! This PR seems OK to me, but likely it will just be the starting point. Obviously feedback is appreciated, and here are some thoughts I had:
- Existing code doesn't seem to use
newoften, I could reduce the usages that I introduce? - Unsure exactly how
manifest::owned::pfmand the elements inmanifest::pfmreally related to each other, should I make changes tomanifest::pfmas well? - I specifically wrote the code to be forward/backward compatible with new flags,
wire_enumdidn't seem to be a perfect fit here especially because these flags don't have the whole byte, just a bit or two at the moment. - The
zeroth_bit_zeroand the<< 6 >> 6shifting don't seem great.. but they're relatively simple so I thought why try something more complicated - I could pull these things out to a
flagsmodule if you'd like, the code is a bit messy all together - Yes my tests test all
u8values, but its not that many. I heard someone once say "why not test them all?" when referring to testing variants on numbers less than like 32 bits, so I just went along with that logic.
Thx!
I just did pretty much an entire re-write of the logic, so a lot of the old comments don't apply.
- Addressed the invariant situation
- Tried to add better documentation comments per the info I had available, bit if you had more background you'd like me to add, let me know!
- updated bit logic
- tests have many more cases
- add getters, and setter on the
ownedPFM
Thanks!