manticore icon indicating copy to clipboard operation
manticore copied to clipboard

[protocol] Parse manifest flags into structs

Open gorup opened this issue 4 years ago • 1 comments

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 new often, I could reduce the usages that I introduce?
  • Unsure exactly how manifest::owned::pfm and the elements in manifest::pfm really related to each other, should I make changes to manifest::pfm as well?
  • I specifically wrote the code to be forward/backward compatible with new flags, wire_enum didn'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_zero and the << 6 >> 6 shifting 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 flags module if you'd like, the code is a bit messy all together
  • Yes my tests test all u8 values, 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!

gorup avatar Sep 08 '21 23:09 gorup

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 owned PFM

Thanks!

gorup avatar Sep 13 '21 21:09 gorup