Fix some more issues #573
Fix some more issues #573
Conversation
|
Can you take particular care checking the RDM sniffer stuff @nomis52 as I don't have a way to test it. |
|
LGTM |
| @@ -327,8 +333,8 @@ void RDMSniffer::DisplayDmxFrame() { | |||
| void RDMSniffer::DisplayAlternateFrame() { | |||
| unsigned int slot_count = m_frame.Size() - 1; | |||
| MaybePrintTimestamp(); | |||
| cout << "SC 0x" << std::hex << std::setw(2) << static_cast<int>(m_frame[0]) | |||
| << " " << std::dec << slot_count << ":" << std::hex; | |||
| cout << "SC " << IntToHexString(m_frame[0]) | |||
There was a problem hiding this comment.
I think you want IntToHexString(m_frame[0], 2) here
There was a problem hiding this comment.
Also IntToHexString is rather expensive. I'd probably stick to what we had here.
There was a problem hiding this comment.
Because m_frame is a uint8_t, it will use that version of IntToHexString surely, passing in 8/4=2. Is the expense just creating the ostringstream? Would making it inline guarantee the compiler optimised it? Alternatively, what if I did a version where you could pass in a pointer to a stream (e.g. cout) like the FormatData function? Also just noticed the old code missed the zero padding bit that IntToHexString does.
|
Were you still planning to make the change I sent you on irc? |
|
Yep, I just got distracted fixing bugs in #575 . |
|
I think this is ready to merge if you're happy @nomis52 . I guess I should switch IntToHexString to use ToHex internally too, and perhaps deprecate it? |
| */ | ||
| template<typename T> | ||
| _ToHex<T> ToHex(T v, bool prefix = true) { | ||
| // TODO(Peter): This may break if we get a type that doesn't have digits |
There was a problem hiding this comment.
That's fine, it's a compile time error. You can remove the TODO
|
LGTM |
No description provided.