Format raw bytes in hex and ascii in manner like Wireshark does

Question

This function formats raw buffer output in manner as Wireshark and many others does:

0x00: 68 65 6C 6C 6F 20 77 6F 72 6C 64 02 6B 68 67 61 |hello world.khga|
0x10: 76 73 64 20 0B 20 0A 05 58 61 73 6A 68 6C 61 73 |vsd . ..Xasjhlas|
0x20: 62 64 61 73 20 6A 61 6C 73 6A 64 6E 13 20 20 30 |bdas jalsjdn. 0|
0x30: 31 32 33 34 35 36 37 38 39 |123456789 |

Please review and assess code quality.

std::string format_hex_payload(const char* payload, size_t payload_len)
{
 const char line_placeholder[] = "0x00: | |";
 size_t number_of_lines = payload_len / 16 + (payload_len%16 > 0);
 size_t sizeof_output = sizeof line_placeholder * number_of_lines;
 char *output = (char*)alloca(sizeof_output); //POSSIBLE REIMPL USE C++RVO std::string out; out.reserve(sizeof_output);
 char *pout = output; //POSSIBLE REIMPL char *pout = out.data();
 const char *p = payload;
 const char *const end = payload + payload_len;
 size_t ascii_offset = strchr(line_placeholder,'|') - line_placeholder + 1; //could be calculated at compile time
 unsigned short offset = 0;
 for(unsigned l=0; l < number_of_lines; l++, offset+=16)
 {
 char* pline_begin = pout;
 char* pline = pout;
 strcpy(pline,line_placeholder);
 pline += sprintf(pline, "0x%02X: ", offset);
 for(unsigned i=0; i<16 && p < end; ++i, ++p){
 pline += sprintf(pline, "%02X ", *p);
 *(pline_begin+ascii_offset+i) = isprint(*p) ? *p : '.';
 }
 *pline=' ';
 pout += sizeof line_placeholder; // move pointer to next line
 pout[-1] = '\n';
 }
 pout[-1] = '0円';
 assert(pout == output + sizeof_output); // sanity check
 return output; //POSSIBLE REIMPL return out;
}

• 1201ProgramAlarm: What happens when offset is >= 256 (more than 16 lines)?
• kyb: @1201ProgramAlarm, nothing criminal, but format will be shifted i.imgur.com/rlMNjc0.png
• Martin R: @kyb: The policy on this site is not to add, remove, or edit code in a question after you've received an answer, because it invalidates existing answers. This is explained in What to do when someone answers. On the other hand, your original question was unclear/off-topic (tagged [C] but invalid C code), which is probably why you code 3 close votes up to now.
• Martin R: @kyb: (Cont.) Following feedback in chat, I have done a rollback the question to revision 3, i.e. the original code but with the [C++] tag, so that both Q and A make sense. I suggest to ask a new question about the C implementation.
• Peilonrayz: Follow up question (Compiles to C rather than C++ now)

Answer 1

1. I think probably the first thing to do is decide whether you're really using C, or really using C++.

   • If you're really using C, then you need to get rid of the std::string, and allocate space for your return differently (e.g., using malloc).

   • If you're really using C++, then I'd at least consider using iostreams and manipulators to do most of the work. They are fairly verbose, but most people using C++ expect to deal with iostreams rather than raw buffers and C-style string manipulation.

2. I think I'd break the code up into a few more functions. For example, I'd probably have format_hex, which would repeatedly call format_line, which might in turn call some format_hex_value (or something on that order) to write out each individual value.

3. I'd probably avoid using the line placeholder, for a couple of reasons. The first (and probably most important) is that as you've done things right now, the last line is (as I see things) somewhat malformed. You've surrounded the ASCII part of the display with | as delimiters, but for that to make sense, you want the delimiter right next to the data, so that part of the last line should look like: |123456789|, so you immediately know there's no more data after the 9 (where your current display looks like there are space characters after the 9).

4. I'd definitely expand the addresses on the left out to at least 4 characters, and maybe more. In theory, you should probably be thinking in terms of 16 characters (allowing 64-bit addresses), but in all honesty I've rarely seen much point in hex dump of multiple gigabytes of data (but I certainly have seen good uses for more than 256 bytes).

Putting those together, one possibility (going the C++ route) might come out something on this order:

#include <cctype>
#include <iostream>
#include <iomanip>
#include <sstream>
struct hex {
 unsigned value;
 int digits;
public:
 hex(unsigned value, int digits) : value(value), digits(digits) { }
 friend std::ostream &operator<<(std::ostream &os, hex const &h) {
 auto oldfill = os.fill('0');
 // Yeah, iostreams get really verbose. Sorry.
 os << std::hex 
 << std::setw(h.digits) 
 << std::uppercase 
 << std::setprecision(h.digits)
 << h.value;
 os.fill(oldfill);
 return os;
 }
};
void fmt_line(std::ostream &os, char const *data, size_t offset, size_t len) { 
 os << hex(offset, 8) << ":";
 for (size_t i=0; i<len; i++) {
 os << " " << hex(data[offset+i], 2);
 }
 os << std::setw((16-len)*3 +2) << "|";
 for (size_t i=0; i<len; i++) {
 char ch = data[offset+i];
 os << (std::isprint((unsigned char)ch) ? ch : '.');
 }
 os << "|\n";
}
std::string format_hex(char const *data, size_t len) { 
 unsigned lines = len/16;
 std::stringstream out;
 for (size_t line=0; line<lines; line++)
 fmt_line(out, data, line*16, 16);
 fmt_line(out, data, lines*16, len%16);
 return out.str();
}
int main() { 
 char input[] = "hello world02円khgavsd \xb \xa\x5Xasjhlasbdas jalsjdn\xd 0123456789";
 std::cout << format_hex(input, sizeof(input));
}

This could use some further improvement. For one thing, it's probably not immediately obvious what some things like: os << std::setw((16-len)*3 +2) << "|"; are really doing, so they could undoubtedly use at least a little more work.