E1.33 cherry pick #1939
E1.33 cherry pick #1939
Conversation
|
This first block is ready for review when you get a chance @kripton ... |
kripton
left a comment
kripton
left a comment
There was a problem hiding this comment.
It all makes sense and I added some inline comments.
I just did a view-code-only review, no compile testing done locally. I trust our actions to verify that it compiles and tests properly and I trust you that it does what you intended ;)
Having said that, it looks good, go ahead 👍 Thanks for your work!
| // masks for the flag fields | ||
| /** | ||
| * @brief This indicates a 20 bit length field (default is 12 bits) | ||
| */ |
There was a problem hiding this comment.
github colors this line in the diff, not sure why?
There was a problem hiding this comment.
I don't see any special colour in mine, just that the whole file is new. 🤷
| uint8_t vhl_flags = static_cast<uint8_t>((size & 0x0f0000) >> 16); | ||
| vhl_flags |= VFLAG_MASK | HFLAG_MASK | DFLAG_MASK; | ||
| if (m_force_length_flag) { | ||
| // TODO(Peter): Should this happen regardless of the force as we're |
Thanks very much!
Yeah that makes sense, that's the level I was expecting really. Plus the code in each of the main PRs has been well tested and I just keep cherry-picking into this from those until it passed the tests!
Hopefully, I certainly appreciate you probably don't want to read all he standards too yourself!
Thanks. Do you want to review my responses/merge that suggestion if it was what you intended or indeed this whole PR @kripton ? Then onto the next one! Was this about right in terms of review size or would you prefer bigger or smaller? |
|
Size was just about right :) A bit more changes / longer PR is okay. You want to merge this before making the next PR? Go ahead! |
2 participants
No description provided.